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.

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

Baseline validation #93

Merged
merged 10 commits into from
Apr 24, 2024
Merged

Conversation

LibraChris
Copy link
Contributor

Add baseline Validation for ARCs in case no specific validation package is referenced

add baseline Validation for ARCs in case no specific validation package is referenced
@LibraChris
Copy link
Contributor Author

@kMutagene Check this out please! I am happy do change anything you say needs a rework.

src/arc-validate/arc-validate.fsproj Show resolved Hide resolved
src/arc-validate/APIs/BaselineValidation.fs Outdated Show resolved Hide resolved
src/arc-validate/APIs/BaselineValidation.fs Outdated Show resolved Hide resolved
src/arc-validate/APIs/BaselineValidation.fs Outdated Show resolved Hide resolved
- Rename BaselineValidation -> ARCSpecification
- Rename isaLight -> V2_Draft
- Outsource the Expect functions from V2_Draft to the ValidateAPI
- Remove metadata section from ARCSpecification
- Remove the confusing checkForParam function
- Revert unneeded Changes to the arc-validate.fsproj
Add Mac specific DS_Store files
Add test for ARCSpecification V2_Draft.
@LibraChris
Copy link
Contributor Author

Hey @kMutagene,
please review the changes made and let me know if they are ok.
I noticed that the way I referenced a file in the tests seem to fail in linux but seems to work in the windows tests. If you let me know what I need to change to fix this I will do it asap.

@kMutagene
Copy link
Member

@LibraChris please double check that path that fails in ci. Something like that failing on linux but passing on windows often means that there is a lowercase/uppercase mistake in the path.

@LibraChris
Copy link
Contributor Author

LibraChris commented Apr 23, 2024

@LibraChris please double check that path that fails in ci. Something like that failing on linux but passing on windows often means that there is a lowercase/uppercase mistake in the path.

You are 100% correct. But strange that it worked on Mac.

Copy link
Member

@kMutagene kMutagene left a comment

Choose a reason for hiding this comment

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

I think we are close to wrapping this up. Upon further inspection i think the specification validation cases should be part of the ARCExpect library, so it could in theory be re-used elsewhere.

src/arc-validate/APIs/ARCSpecification.fs Outdated Show resolved Hide resolved
src/arc-validate/APIs/ARCSpecification.fs Outdated Show resolved Hide resolved
tests/arc-validate.Tests/ARCSpecification.fs Outdated Show resolved Hide resolved
src/arc-validate/APIs/ARCSpecification.fs Outdated Show resolved Hide resolved
let runSummery = ARCExpect.Execute.Validation cases

//false None (ParseResults<ARCValidate.CLIArguments.ValidateArgs>.Empty)
Expect.isTrue
Copy link
Member

Choose a reason for hiding this comment

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

As you can see in the case of the other tests, in this test project we do not test library functions but the execution of the tool. In this case, a test for arc-validate validate is needed. This test however is completely valid once it has been moved into ARCExpect.Tests

- Move ARCSpecification into ARCExpect
- Move Tests and relevant test files in respect
- Rename ARCValidate.ARCSpecification. V2_Draft. testCases ->  ARCExpect.ARCSpecification. V2_Draft .validationCases
- Remove internal keyword from V2_Draft modules
- Fixed typos
@LibraChris
Copy link
Contributor Author

Hey @kMutagene , I addressed the changes you requested. Let me know if it looks good.

src/arc-validate/APIs/ValidateAPI.fs Outdated Show resolved Hide resolved
tests/ARCExpect.Tests/ARCSpecification.fs Outdated Show resolved Hide resolved
tests/arc-validate.Tests/arc-validate.Tests.fsproj Outdated Show resolved Hide resolved
- Fix Error Message in tests
- Change Usage of Validation Pipeline to its components
- Removed redundant references in fsproj
@LibraChris
Copy link
Contributor Author

@kMutagene How about now?

let outDirBadge = System.IO.Path.Combine(root, "ARC_specification_V2_Draft.svg")
let outDirResXml =System.IO.Path.Combine(root, "ARC_specification_V2_Draft.xml")

ARCExpect.Execute.ValidationPipeline(jUnitPath=outDirResXml, badgePath=outDirBadge,labelText="ARC specification V2_Draft") cases
Copy link
Member

Choose a reason for hiding this comment

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

This line (ARCExpect.Execute.ValidationPipeline) still has to go. Now you are validating and writing 2 result files 2 times each.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry @kMutagene , I didn´t notice that I left it in. Now its removed.

Copy link
Member

@kMutagene kMutagene left a comment

Choose a reason for hiding this comment

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

🎉

@kMutagene kMutagene merged commit 4d70e78 into nfdi4plants:main Apr 24, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants