Update exec_cfg_check to use config source path relative to the hub root
#142
Conversation
There was a problem hiding this comment.
Thanks for catching and fixing this!
In all honesty I'm finding it harder to follow what's going on in the tests though. I prefer having tangible config files that can be inspected easily online without having to pick through and figure out what the stand up code is doing.
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).
I think at least some comments to explain what going is needed in the stand up function.
| }) | ||
| }) | ||
|
|
||
| expect_snapshot(print(res)) |
There was a problem hiding this comment.
It's not that informative to take a snapshot of a generically named object as in the snapshot, it's not clear what the snapshot refers to. I'd prefer if the snapshot were of either:
- more complete code generating the snapshot
- a more descriptive name for the contents of the object being snapshotted
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. |
This reverts commit 15a0939.
Using the dynamic test path meant that we could not test these files as they existed in a hub. With this pattern, we can copy the test files over instead of having to do the cumbersome programmatic generation. Less code == better code
|
No worries! BTW, could you add a note in |
Done! It's ready for another pass, @annakrystalli 😃 |
There was a problem with
exec_cfg_checkwhere it was assuming that the working directory was the same directory as the hub directory. This PR fixes that issue by pullinghub_pathfrom 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:
I added an extra test helper to do the following:
src/validations/Rin the copy of the hubvalidations.ymlfile with the correct parametersThis will fix #141