Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add custom missing mandatory UFO path error types & missing mandatory UFO path checks #146

Merged
merged 11 commits into from
Jul 28, 2021
Merged

Add custom missing mandatory UFO path error types & missing mandatory UFO path checks #146

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e37bc28
[error] add custom Error::MissingFile and Error::MissingUFODir error …
chrissimpkins Jul 27, 2021
8670748
[font] perform UFO dir path existence validity check, raise Error::Mi…
chrissimpkins Jul 27, 2021
f15320d
refactor Error::MissingLayerContents error to Error::MissingFile error
chrissimpkins Jul 27, 2021
7b6cb1e
Error::MissingUFODir --> Error::MissingUfoDir
chrissimpkins Jul 27, 2021
6c9e817
add test UFO source files with missing metainfo.plist
chrissimpkins Jul 27, 2021
d9d26c4
[font] add missing metainfo.plist file check
chrissimpkins Jul 27, 2021
3f12d08
add missing mandatory layercontents.plist file test + test sources
chrissimpkins Jul 27, 2021
a418ef2
add glyphs and background glyphs dir mandatory contents.plist file ch…
chrissimpkins Jul 27, 2021
0974b9a
remove debugging println statements
chrissimpkins Jul 27, 2021
a25963b
pare down UFO test files
chrissimpkins Jul 28, 2021
b1c17ab
[font] update unit test approach to use matches! macro tests on Error…
chrissimpkins Jul 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ pub enum Error {
MissingDefaultLayer,
MissingLayer(String),
DuplicateLayer(String),
MissingLayerContents,
InvalidColor(InvalidColorString),
DuplicateGlyph {
layer: String,
Expand All @@ -44,6 +43,8 @@ pub enum Error {
ExpectedPositiveValue,
#[cfg(feature = "kurbo")]
ConvertContour(ErrorKind),
MissingFile(String),
MissingUfoDir(String),
}

/// An error representing a failure to validate UFO groups.
Expand Down Expand Up @@ -160,9 +161,6 @@ impl std::fmt::Display for Error {
Error::MissingDefaultLayer => write!(f, "Missing default ('glyphs') layer."),
Error::DuplicateLayer(name) => write!(f, "Layer name '{}' already exists.", name),
Error::MissingLayer(name) => write!(f, "Layer name '{}' does not exist.", name),
Error::MissingLayerContents => {
write!(f, "Missing required 'layercontents.plist' file.")
}
Error::DuplicateGlyph { layer, glyph } => {
write!(f, "Glyph named '{}' already exists in layer '{}'", glyph, layer)
}
Expand Down Expand Up @@ -194,6 +192,12 @@ impl std::fmt::Display for Error {
Error::ExpectedPositiveValue => {
write!(f, "PositiveIntegerOrFloat expects a positive value.")
}
Error::MissingFile(path) => {
write!(f, "missing required {} file", path)
}
Error::MissingUfoDir(path) => {
write!(f, "{} directory was not found", path)
}
#[cfg(feature = "kurbo")]
Error::ConvertContour(cause) => write!(f, "Failed to convert contour: '{}'", cause),
}
Expand Down
72 changes: 71 additions & 1 deletion src/font.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,14 @@ impl Font {
}

fn load_impl(path: &Path, request: DataRequest) -> Result<Font, Error> {
if !path.exists() {
return Err(Error::MissingUfoDir(path.display().to_string()));
}

let meta_path = path.join(METAINFO_FILE);
if !meta_path.exists() {
return Err(Error::MissingFile(meta_path.display().to_string()));
}
let mut meta: MetaInfo = plist::from_file(meta_path)?;

let lib_path = path.join(LIB_FILE);
Expand Down Expand Up @@ -393,7 +400,7 @@ fn load_layers(
) -> Result<LayerSet, Error> {
let layercontents_path = ufo_path.join(LAYER_CONTENTS_FILE);
if meta.format_version == FormatVersion::V3 && !layercontents_path.exists() {
return Err(Error::MissingLayerContents);
return Err(Error::MissingFile(layercontents_path.display().to_string()));
}
LayerSet::load(ufo_path, glyph_names)
}
Expand Down Expand Up @@ -438,6 +445,69 @@ mod tests {
assert_eq!(font_obj.features.unwrap(), "# this is the feature from lightWide\n");
}

#[test]
fn loading_invalid_ufo_dir_path() {
let path = "totally/bogus/filepath/font.ufo";
let font_load_res = Font::load(path);
match font_load_res {
Ok(_) => panic!("unxpected Ok result"),
Err(Error::MissingUfoDir(_)) => (), // expected value
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assert_eq!(font_load_res, Err(Error::MissingUfoDir(...))) or something maybe?

Copy link
Collaborator Author

@chrissimpkins chrissimpkins Jul 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried. Compiler won't let me perform equality comparisons on the Result Error type

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things:

As written, I would do this as,

assert!(matches!(font_load_res, Err(Error::MissingUfoDir(_))));

but there's probably a better option: explicitly testing for a panic with the should_panic attribute.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but there's probably a better option: explicitly testing for a panic with the should_panic attribute.

Ah nice, I didn't realize that you can add an expected string with should_panic. I tried this approach but I can't elicit a panic from the new custom Error. The attribute doesn't seem to catch errors like it does panics. I see this in the docs that you linked:

You can’t use the #[should_panic] annotation on tests that use Result<T, E>. Instead, you should return an Err value directly when the test should fail.

Let me tinker with the test Result approach a bit. If I can't find a better solution, I'll transition things over the matches! macro approach above.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would work if you just unwrap the Result?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(but matches! is totally reasonable too)

Copy link
Collaborator Author

@chrissimpkins chrissimpkins Jul 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the use of a Result return type in unit tests is good if you expect an Ok() but there isn't a way to expect an Err. We could return a test error when the function being tested returns Ok 💥 but that seems worse than the match pattern matching approach :)

I went with the matches! macro. That allows us to confirm the custom error type and works well!

b1c17ab

_ => panic!("incorrect error type returned"),
}
}

#[test]
fn loading_missing_metainfo_plist_path() {
// This UFO source does not have a metainfo.plist file
// This should raise an error
let path = "testdata/ufo/Tester-MissingMetaInfo.ufo";
let font_load_res = Font::load(path);
match font_load_res {
Ok(_) => panic!("unxpected Ok result"),
Err(Error::MissingFile(_)) => (), // expected value
_ => panic!("incorrect error type returned"),
}
}

#[test]
fn loading_missing_layercontents_plist_path() {
// This UFO source does not have a layercontents.plist file
// This should raise an error
let path = "testdata/ufo/Tester-MissingLayerContents.ufo";
let font_load_res = Font::load(path);
match font_load_res {
Ok(_) => panic!("unxpected Ok result"),
Err(Error::MissingFile(_)) => (), // expected value
_ => panic!("incorrect error type returned"),
}
}

#[test]
fn loading_missing_glyphs_contents_plist_path() {
// This UFO source does not have contents.plist in the default glyphs
// directory. This should raise an error
let path = "testdata/ufo/Tester-MissingGlyphsContents.ufo";
let font_load_res = Font::load(path);
match font_load_res {
Ok(_) => panic!("unxpected Ok result"),
Err(Error::MissingFile(_)) => (), // expected value
_ => panic!("incorrect error type returned"),
}
}

#[test]
fn loading_missing_glyphs_contents_plist_path_background_layer() {
// This UFO source has a contents.plist in the default glyphs directory
// but not in the glyphs.background directory. This should raise an error
let path = "testdata/ufo/Tester-MissingGlyphsContents-BackgroundLayer.ufo";
let font_load_res = Font::load(path);
match font_load_res {
Ok(_) => panic!("unxpected Ok result"),
Err(Error::MissingFile(_)) => (), // expected value
_ => panic!("incorrect error type returned"),
}
}

#[test]
fn data_request() {
let path = "testdata/mutatorSans/MutatorSansLightWide.ufo";
Expand Down
3 changes: 3 additions & 0 deletions src/layer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,9 @@ impl Layer {
names: &NameList,
) -> Result<Layer, Error> {
let contents_path = path.join(CONTENTS_FILE);
if !contents_path.exists() {
return Err(Error::MissingFile(contents_path.display().to_string()));
}
// these keys are never used; a future optimization would be to skip the
// names and deserialize to a vec; that would not be a one-liner, though.
let contents: BTreeMap<GlyphName, PathBuf> = plist::from_file(contents_path)?;
Expand Down
38 changes: 38 additions & 0 deletions testdata/ufo/Tester-MissingGlyphsContents-BackgroundLayer.ufo/fontinfo.plist
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
<?xml version='1.0' encoding='UTF-8'?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
<key>ascender</key>
<integer>750</integer>
<key>capHeight</key>
<integer>750</integer>
<key>descender</key>
<integer>-250</integer>
<key>familyName</key>
<string>Tester MissingGlyphsContents</string>
<key>guidelines</key>
<array/>
<key>postscriptBlueValues</key>
<array/>
<key>postscriptFamilyBlues</key>
<array/>
<key>postscriptFamilyOtherBlues</key>
<array/>
<key>postscriptOtherBlues</key>
<array/>
<key>postscriptStemSnapH</key>
<array/>
<key>postscriptStemSnapV</key>
<array/>
<key>styleName</key>
<string>Regular</string>
<key>unitsPerEm</key>
<integer>1000</integer>
<key>versionMajor</key>
<integer>1</integer>
<key>versionMinor</key>
<integer>0</integer>
<key>xHeight</key>
<integer>500</integer>
</dict>
</plist>
8 changes: 8 additions & 0 deletions ...ta/ufo/Tester-MissingGlyphsContents-BackgroundLayer.ufo/glyphs.background/layerinfo.plist
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<?xml version='1.0' encoding='UTF-8'?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
<key>color</key>
<string>0,0.8,0.2,0.7</string>
</dict>
</plist>
8 changes: 8 additions & 0 deletions testdata/ufo/Tester-MissingGlyphsContents-BackgroundLayer.ufo/glyphs/contents.plist
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<?xml version='1.0' encoding='UTF-8'?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
<key>space</key>
<string>space.glif</string>
</dict>
</plist>
8 changes: 8 additions & 0 deletions testdata/ufo/Tester-MissingGlyphsContents-BackgroundLayer.ufo/glyphs/layerinfo.plist
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<?xml version='1.0' encoding='UTF-8'?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
<key>color</key>
<string>1,0.75,0,0.7</string>
</dict>
</plist>
7 changes: 7 additions & 0 deletions testdata/ufo/Tester-MissingGlyphsContents-BackgroundLayer.ufo/glyphs/space.glif
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<?xml version='1.0' encoding='UTF-8'?>
<glyph name="space" format="2">
<advance width="500"/>
<unicode hex="0020"/>
<outline>
</outline>
</glyph>
14 changes: 14 additions & 0 deletions testdata/ufo/Tester-MissingGlyphsContents-BackgroundLayer.ufo/layercontents.plist
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<?xml version='1.0' encoding='UTF-8'?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<array>
<array>
<string>foreground</string>
<string>glyphs</string>
</array>
<array>
<string>background</string>
<string>glyphs.background</string>
</array>
</array>
</plist>
Loading