hubverse-org / hubValidations

Testing framework for hubverse hub validations
https://hubverse-org.github.io/hubValidations/
Other
1 stars 4 forks source link

Update `exec_cfg_check` to use config source path relative to the hub root #142

Closed zkamvar closed 1 month ago

zkamvar commented 1 month ago

There was a problem with exec_cfg_check where it was assuming that the working directory was the same directory as the hub directory. This PR fixes that issue by pulling hub_path from the caller's environment and using that to create a valid path.

With this fix, however I had to also rewrite the tests because a lot of them were written in a way that worked very well to test that the execution order would be respected when the custom checks were called, but they did not test that the checks actually lived inside of the hub, which did pose a bit of a security concern because you effectively would end up with a situation where you could set an arbitrary URL as the source for the script:

source: https://example.com/check.R

I added an extra test helper to do the following:

  1. create a copy of a test hub
  2. copy a check script to src/validations/R in the copy of the hub
  3. create the validations.yml file with the correct parameters

This will fix #141

github-actions[bot] commented 1 month ago

🚀 Deployed on https://6723a21eb8988b34e933e40c--hubvalidations-pr-previews.netlify.app

zkamvar commented 1 month ago

For me it would have been quicker and easier to follow if the deleted test yaml files were being used to configure each test instead of programmatically generating validations.yml files and the stand up function just took an argument that specified which .yml file to copy into the test hub. Then, once all contents of testthat::test_path("testdata/src/R/") were copied into the test hubs src/validations/R/) directory, the validations.yml file could be used to configure which file to source (as we do in real hubs).

Understood. I was having a bear of a time with this. In short, I got confused and hurt myself in my confusion. I'll rejig.

annakrystalli commented 1 month ago

No worries! BTW, could you add a note in NEWS.md too about the bug fix?

zkamvar commented 1 month ago

No worries! BTW, could you add a note in NEWS.md too about the bug fix?

Done! It's ready for another pass, @annakrystalli :smiley: