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

tests: use asset files #62

Merged
merged 7 commits into from
Nov 24, 2022
Merged

tests: use asset files #62

merged 7 commits into from
Nov 24, 2022

Conversation

zargot
Copy link
Collaborator

@zargot zargot commented Nov 10, 2022

Replace hardcoded parts/schemas in tests with parts/schemas loaded from the assets directory.

The remaining asset files will be incorporated in #69.

@zargot zargot linked an issue Nov 10, 2022 that may be closed by this pull request
Copy link
Collaborator

@yulric yulric left a comment

Choose a reason for hiding this comment

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

  1. I'm not sure what happened but it looks like the pprint_csv_file function is no longer printing to console? Is that happening for you?
  2. Can you update all the invalid_category rule files (assets and qmd) with the changes you made to the parts sheet?

Copy link
Collaborator Author

@zargot zargot left a comment

Choose a reason for hiding this comment

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

edit: nvm.

Copy link
Collaborator

@yulric yulric left a comment

Choose a reason for hiding this comment

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

  1. For each test, can you use all the datasets and error report in the assets directory? I put them in there so we can test what I hope are all the cases.
  2. Can you say why the addL2 part was removed from the missing mandatory column parts sheet? I put it in there to make sure optional columns do not have a mandatory rule added to them

src/odm_validation/part_tables.py Outdated Show resolved Hide resolved
@DougManuel
Copy link
Contributor

For each test, can you use all the datasets and error report in the assets directory? I put them in there so we can test what I hope are all the cases.

I am not sure if my comment is the same as @yulric, but I too was wondering about the tests.

  • I wondered whether the test for each rule should go in the folder for the validation rule. This is a small point. I also see the value of keeping tests together.
  • There was hard coded test data in the test code. Was this intentional. I was expecting you would use the invalid data in the corresponding validation-rules folder.

@DougManuel
Copy link
Contributor

I suggest deleting the metadata folder and putting the validation-rule-list.csv into the validation-rules folder. Just in the main directory, validation-rules/validation-rule-list.csv.

If you do want to keep the metadata folder, I'd rename it because really much of the repo is metadata -- so it is hard to know what to expect is in this specific folder.

@DougManuel
Copy link
Contributor

This may need a discussion, but hear me out. What about moving the validation .qmd files into their respective folders within assets?

We've got a small problem that we have to 'valiation-rules' folders. One in assets and the other in the parent directory.

As a user, it would be nice to be able to go into rule in assets/validtion-rules and find a qmd that helps explain what all the assets are.

Copy link
Contributor

@DougManuel DougManuel left a comment

Choose a reason for hiding this comment

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

The changes look excellent.

See my comments about the organization of the assets.

  1. It is tempting to validation rule .qmd, tests into the single asset folder -- or at least together in a folder.

Then put the validation rule README and validation-rule-list.csv in the parent directory of assets/validation-rules.

  1. as an aside, tests/common.py may be better placed in src and renamed something like test_utils.

  2. I noted that some parts have names that have changed in the most common part table. We may want update partID names when we have a version 2 completed, especially if we have the schemaVersion indicate '2.0.0' as it does currently. The validation examples should work without updating the names because they are all self-contained -- perfect. However, there are a few sections of the rules that may break, such as reference to the missingness partIDs, which have changed in the most recent parts table.

@zargot zargot closed this Nov 21, 2022
@zargot zargot reopened this Nov 21, 2022
@zargot
Copy link
Collaborator Author

zargot commented Nov 21, 2022

  • I wondered whether the test for each rule should go in the folder for the validation rule. This is a small point. I also see the value of keeping tests together.

This is a good question. The tests aren't part of the main package. I would suggest moving test assets into /tests/assets, but they are also used by the documentation which will go in the /docs folder (eventually), so I think it makes the most sense to keep it as is (/assets), since it's a shared resource.

  • There was hard coded test data in the test code. Was this intentional. I was expecting you would use the invalid data in the corresponding validation-rules folder.

Will fix in #69

@zargot
Copy link
Collaborator Author

zargot commented Nov 21, 2022

I suggest deleting the metadata folder and putting the validation-rule-list.csv into the validation-rules folder. Just in the main directory, validation-rules/validation-rule-list.csv.

If you do want to keep the metadata folder, I'd rename it because really much of the repo is metadata -- so it is hard to know what to expect is in this specific folder.

I agree with doing something about the metadata folder. I'm about to move /validation-rules to /docs/validation-rules, and the rule-list csv is more of an asset than documentation, so maybe move it to /assets? I'll make a new issue for this discussion (#71).

@zargot
Copy link
Collaborator Author

zargot commented Nov 21, 2022

This may need a discussion, but hear me out. What about moving the validation .qmd files into their respective folders within assets?

We've got a small problem that we have to 'valiation-rules' folders. One in assets and the other in the parent directory.

As a user, it would be nice to be able to go into rule in assets/validtion-rules and find a qmd that helps explain what all the assets are.

I'm about to move /validation-rules to /docs/validation-rules (in #15), which may make this less confusing for users. The assets aren't really meant to be read directly, so I think it's fine.

@zargot
Copy link
Collaborator Author

zargot commented Nov 21, 2022

The changes look excellent.

See my comments about the organization of the assets.

  1. It is tempting to validation rule .qmd, tests into the single asset folder -- or at least together in a folder.

Then put the validation rule README and validation-rule-list.csv in the parent directory of assets/validation-rules.

I think my previous comments cover this.

  1. as an aside, tests/common.py may be better placed in src and renamed something like test_utils.

I think src/odm_validation and tests are supposed to be independent, so I would prefer to keep test-specific code in the tests dir.

  1. I noted that some parts have names that have changed in the most common part table. We may want update partID names when we have a version 2 completed, especially if we have the schemaVersion indicate '2.0.0' as it does currently. The validation examples should work without updating the names because they are all self-contained -- perfect. However, there are a few sections of the rules that may break, such as reference to the missingness partIDs, which have changed in the most recent parts table.

Yeah, we should update partIDs when releasing v2 (or rc2). I've tried to keep everything consistent with the v2-rc1 names, and specified the '-rc.1' suffix in the version strings. There may still be some inconsistencies, but I'll fix them when I see them.

@zargot zargot merged commit 6c65fcd into main Nov 24, 2022
@zargot zargot deleted the 58-refactor-test-assets branch November 24, 2022 17:50
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.

Update tests to use (parts/schema-)files in assets folder
3 participants