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

Fix errors in test model output file #184

Merged
merged 2 commits into from
Dec 18, 2024

Conversation

annakrystalli
Copy link
Member

@annakrystalli annakrystalli commented Dec 17, 2024

As described in #182, when correcting s3 class tests, @zkamvar discovered a test that was actually failing when it should in fact be passing. The reason was an error in a test file which included trailing zeros in quantile output type ids in the test csv file. Once that was corrected the test passes.

This PR resolves the issue #183 by:

  • Removing trailing zeros from the quantile output type id values in the csv file. Re-writing also caused empty cells to now appear as NAs but that is equally valid representation of missing values so I've left it.
  • Removing test skip and confirming test passes
  • Although not functionally important nor user facing, I also updated the related task.json config file to remove trailing zeros from quantile output type id value specification and conform to the best practice we advocate https://hubverse.io/en/latest/quickstart-hub-admin/tasks-config.html#setting-up-quantile
  • I also checked the 2 other parquet test files and they did not contain trailing zeros so no correction required.

@zkamvar I branched off from and made this PR into your test correction PR so that I could remove the test skip. I can confirm the test passes locally and if you were happy to merge into your PR, you could double check the tests pass on there before merging into main. If not let me know when #182 is merged and I'll synch this PR with main and redirect it

When correcting s3 class tests, @zkamvar discovered a test that was actually failing when it should in fact be passing. The reason was an error in the test files which included trailing zeros in quantiles. Once that was corrected the test passed.
While it has no actual effect on tests and is only an internal test file, it goes against the best practice we advocate: https://hubverse.io/en/latest/quickstart-hub-admin/tasks-config.html#setting-up-quantile
@zkamvar zkamvar merged commit 1268425 into znk/s3-class-tests/180 Dec 18, 2024
@zkamvar zkamvar deleted the ak/update-check-expectation/183 branch December 18, 2024 16:43
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 expectation for skipped check_tbl_values_required test
2 participants