Skip to content
New issue

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

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

Already on GitHub? # to your account

allow more than one test file for a license #2635

Closed
wants to merge 1 commit into from

Conversation

xsuchy
Copy link
Collaborator

@xsuchy xsuchy commented Dec 17, 2024

For now, we allow only one test file for license.
Various licenses have variation. And we are adding more and more markup variations. But we do not have tests for them.

I propose to test not just
test/simpleTestForGenerator/${LICENSE}.txt
but also
test/simpleTestForGenerator/${LICENSE}.*.txt
files. If they exists.

Adding test/simpleTestForGenerator/BSD-3-Clause.edl-v10.txt as an example.

This include enhancement to test-one-license only. Needs to be followed by enhancement of LicenseRDFAGenerator in licenselistpublisher.

The output of test-one-license looks like:

$ ./test-one-license BSD-3-Clause
Testing test/simpleTestForGenerator/BSD-3-Clause.txt
WARNING: sun.reflect.Reflection.getCallerClass is not supported. This will impact performance.
License BSD 3-Clause "New" or "Revised" License passed
Testing test/simpleTestForGenerator/BSD-3-Clause.edl-v10.txt
WARNING: sun.reflect.Reflection.getCallerClass is not supported. This will impact performance.
License BSD 3-Clause "New" or "Revised" License passed

For now, we allow only one test file for license.
Various licenses have variation. And we are adding more and more markup variations.
But we do not have tests for them.

I propose to test not just
  test/simpleTestForGenerator/${LICENSE}.txt
but also
  test/simpleTestForGenerator/${LICENSE}.*.txt
files. If they exists.

Adding test/simpleTestForGenerator/BSD-3-Clause.edl-v10.txt as an example.

This include enhancement to `test-one-license` only. Needs to be
followed by enhancement of `LicenseRDFAGenerator` in `licenselistpublisher`.

The output of `test-one-license` looks like:
$ ./test-one-license BSD-3-Clause
Testing test/simpleTestForGenerator/BSD-3-Clause.txt
WARNING: sun.reflect.Reflection.getCallerClass is not supported. This will impact performance.
License BSD 3-Clause "New" or "Revised" License passed
Testing test/simpleTestForGenerator/BSD-3-Clause.edl-v10.txt
WARNING: sun.reflect.Reflection.getCallerClass is not supported. This will impact performance.
License BSD 3-Clause "New" or "Revised" License passed
@goneall
Copy link
Member

goneall commented Dec 17, 2024

This include enhancement to test-one-license only. Needs to be followed by enhancement of LicenseRDFAGenerator in licenselistpublisher.

We really should implement in the LicenseListPublisher - more efficient and will work when testing all the licenses.

I agree with the proposed naming standard for the tests.

Turns out @swinslow and I are currently making updates to the LicenseListPublisher - depending on the timing of the next release of the license list, I may be able to update the publisher before release.

@goneall
Copy link
Member

goneall commented Dec 17, 2024

In looking back at the LicenseListPublisher code, there is already an implementation of a tester that supports multiple files.

The file / directory naming format is {license-id}/(license|header|exception)/(good|bad)/{test-id}.txt

See this comment for details.

We could (re) use this code if that directory / file structure is OK. Otherwise, we'll need to implement a new tester for this format.

I also discovered that some refactoring is in order - so I won't be able to get a change in for the next release of the LicenseListPublisher.

@xsuchy
Copy link
Collaborator Author

xsuchy commented Dec 18, 2024

That sounds appealing. Does LicenseRDFAGenerator support this structure, too?

@goneall
Copy link
Member

goneall commented Dec 18, 2024

That sounds appealing. Does LicenseRDFAGenerator support this structure, too?

The functionality is not currently being used, but it should only be a few lines to code to include it. I think we can just add the tests without changing the command line since the new tests will follow a specific directory and file naming structure. If we find the pattern {license-id}/(license|header|exception)/(good|bad)/{test-id}.txt we run the tests. If we find just a {license-id}.txt pattern, we can run the simple single file test.

If there is general agreement to use the {license-id}/(license|header|exception)/(good|bad)/{test-id}.txt pattern in this repo, I can make the changes to the license list publisher.

@swinslow
Copy link
Member

@goneall I think this is great, and I'm +1 to the format you described. Sounds like it's fully backwards-compatible with current practice if a single matching file ID means that it'll be treated as the sole test.

Just to ask (and apologies if I missed it above), is there a difference in how filenames containing good vs. bad will be treated as test cases? E.g., would bad ones fail the test if the XML does match the bad test text?

@goneall
Copy link
Member

goneall commented Dec 19, 2024

would bad ones fail the test if the XML does match the bad test text?

Correct - the "bad" test case is a negative test case - so if you want to make sure a text does not match, you can put it in the "bad" directory.

@goneall
Copy link
Member

goneall commented Dec 20, 2024

I added PR #2639 which supports multiple files.

@xsuchy
Copy link
Collaborator Author

xsuchy commented Jan 14, 2025

Closing in favour of the @goneall work.

@xsuchy xsuchy closed this Jan 14, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants