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

Extend test cases #25

Merged
merged 3 commits into from
May 14, 2018
Merged

Extend test cases #25

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
721a88b
Extract tests out to separate test file
Nemo157 May 7, 2018
f2a6752
Extend test cases
Nemo157 May 7, 2018
6570927
Use AGPL-1.0+ instead of MIT+ in tests
Nemo157 May 14, 2018
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
32 changes: 1 addition & 31 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use self::LicenseExpr::*;
pub enum LicenseExpr<'a> {
License(&'a str),
Exception(&'a str),
And, Or, With,
And, Or, With,
}

impl<'a> fmt::Display for LicenseExpr<'a> {
Expand Down Expand Up @@ -76,33 +76,3 @@ pub fn validate_license_expr(license_expr: &str) -> Result<(), ParseError> {
pub fn license_version() -> &'static str {
spdx::VERSION
}

#[cfg(test)]
mod tests {

#[test]
fn single_license() {
assert!(super::validate_license_expr("MIT").is_ok());
}

#[test]
fn compound_license() {
assert!(super::validate_license_expr("GPL-3.0+ WITH Classpath-exception-2.0 OR MIT AND AAL")
.is_ok());
}

#[test]
fn fails_invalid_license() {
assert!(super::validate_license_expr("asdfghjkl").is_err());
assert!(super::validate_license_expr("MIT AND qwerty").is_err())
}

#[test]
fn fails_incorrect_structure() {
assert!(super::validate_license_expr("WITH").is_err());
assert!(super::validate_license_expr("MIT OR WITH").is_err());
assert!(super::validate_license_expr("MIT AND Classpath-exception-2.0").is_err());
assert!(super::validate_license_expr("Classpath-exception-2.0").is_err());
}

}
72 changes: 72 additions & 0 deletions tests/validate_license_expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
extern crate license_exprs;

use license_exprs::validate_license_expr;

macro_rules! assert_licenses_ok {
($($license:expr),+ $(,)+) => {{
$(assert!(validate_license_expr($license).is_ok());)+
}}
}

macro_rules! assert_licenses_err {
($($license:expr),+ $(,)+) => {{
$(assert!(validate_license_expr($license).is_err());)+
}}
}

#[test]
fn single_license() {
assert_licenses_ok! {
"MIT",
"MIT ",
" MIT",
" MIT ",
" MIT ",
"MIT+",
Copy link
Contributor

Choose a reason for hiding this comment

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

While this is technically legal with the SPDX license expression syntax, I'd like to have future SPDX versions be able to error when + is applied to a license for which it makes no sense. I don't think the SPDX License List has any non-deprecated identifiers where + makes sense, and GPL-3.0+ and similar are deprecated short identifiers even without a + operator. So for testing +, I'd prefer using AGPL-1.0+ (and AGPL-1.0 + where you have MIT + below). The AGPL-1.0 identifier is deprecated in 3.1, but it exercises the + operator in a way that makes legal sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to have to read a bit more around this, and see what other libraries are doing. I agree that MIT+ doesn't make sense, but as you say it's technically legal.

I'm thinking about how to add warnings to the API as well, for things like deprecated identifiers, so adding a + onto a license where it doesn't make sense could be a warning until the spec explicitly denies it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking about how to add warnings to the API as well, for things like deprecated identifiers, so adding a + onto a license where it doesn't make sense could be a warning until the spec explicitly denies it.

That would be great (links to similar comments from here).

On the MIT+ vs. AGPL-1.0+ front, have you settled on a preference? I prefer AGPL-1.0+ because I weight "makes legal sense" more than "based on a non-deprecated identifier". But I don't feel stronlgy enough about it to hold up this PR, which looks good to me otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the tests to use AGPL-1.0+

}
}

#[test]
fn compound_license() {
assert_licenses_ok! {
"GPL-3.0+ WITH Classpath-exception-2.0 OR MIT AND AAL",
"MIT AND Apache-2.0",
"MIT OR Apache-2.0",
"GPL-3.0+ WITH Classpath-exception-2.0",
"MIT AND Apache-2.0",
}
}

#[test]
fn fails_invalid_license() {
assert_licenses_err! {
"asdfghjkl",
"MIT AND qwerty",
}
}

#[test]
fn fails_incorrect_structure() {
assert_licenses_err! {
"()",
"(MIT",
"MIT)",
"MIT Apache-2.0",
"MIT and Apache-2.0",
"MIT or Apache-2.0",
"GPL-3.0+ with Classpath-exception-2.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

There's some discussion about making operators case insensitive in spdx/spdx-spec#63, but the 2.1 ABNF requires uppercase operators. So I'm fine requiring failures here for now, but we may need to revisit these if the SPDX discussion results in them (or license expressions as a whole) becoming case insensitive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I presume that can be done as a non-breaking enhancement to the spec so the same can happen here.

"MIT (MIT AND MIT)",
"(MIT AND MIT) MIT",
"and Apache-2.0",
"(MIT and Apache-2.0 and )",
"MIT xor Apache-2.0",
"WITH",
"MIT OR WITH",
// "MIT WITH", // TODO: Incorrectly marked as valid
// "MIT AND", // TODO: Incorrectly marked as valid
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a Rust analog to TAP's TODO tests for marking expected test failures? It would be nice to report these known failures as non-fatal warnings in the test results and be prompted to remove the TODO once we fix the backing implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not in the default test runner that I know of. I'm hopefully going to fix these errors soon when I work on parsing to an actual syntax tree, which will also involve adding a whole bunch more test cases, so I think keeping them as just comments for now should be ok.

"MIT AND Classpath-exception-2.0",
"Classpath-exception-2.0 WITH MIT",
"Classpath-exception-2.0",
"MIT +",
}
}