-
-
Notifications
You must be signed in to change notification settings - Fork 10
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
stable and devel snapshot testing #235
Conversation
@insightsengineering/nest-sme please kindly have a look and share your thoughts on the debatable stuff I outlined above. I'll keep it as a draft until we got an agreement of the way forward. I would also appreciate if you can double-check the snapshots that I got. Docker is complaining for some reason and this is a result of a local install and run. I'm not super sure about this one. |
@pawelru regarding graph testing: the SME team (in discussion with the IDR team) agreed to utilize This is because, aside from the differences that come from different local setups, there are extremely small differences in placement coordinates for different elements of the graph snapshots that are detected when integration tests are run. The IDR team suggested that it would be easier to just ignore these tests during checks, since there is currently no way to enable a tolerance for these minor differences with the format of the image files returned. |
@pawelru what is docker complaining about? I think it is very good; as far as it is possible to check where the snapshots come from somehow smoothly, it is fine with me. As Emily wrote, the graphs for now should be skipped on ci |
I have tried the alternative approach of separate test file for each article and this is quite nice. As a result we have:
The drawback is that every time we move articles around or create a new one - we need to create appropriate test directory. I have submitted a helper script that does it automatically. I hope devs will be able to find and use it if needed. |
OK I think this is ready for review now! |
The rationale: Each modification of an article should be tested against both stable and devel in the feature branch before the merge. Currently this is happening only after merge. Key changes: - moved `r-cmd-stable` from "docs" workflow into "check" - `r-cmd-stable` will inherit triggers from that workflow and became executed in the PR - removed `r-cmd-stable-notification` as `r-cmd-stable` will be executed in the PR UPDATE: actually I think it's incorrect. We might also need `r-cmd-stable` (alongside notifications) to be executed on a schedule. - `publish-stable` job (which renders and publish catalog) is unconditional to `r-cmd-stable`. This is good because `r-cmd-stable` is covered already inside PR. Small enhancements: - remove `enforce-note-blocklist` with its `note-blocklist` because this is a simple and dummy package I was also thinking about renaming existing `r-cmd` into `r-cmd-devel` but decided to keep it as is. Please note this PR targets a feature branch and not main.
Unit Tests Summary338 tests 0 ✅ 55s ⏱️ Results for commit 52b286e. ♻️ This comment has been updated with latest results. |
Unit Test Performance DifferenceTest suite performance difference
Additional test case details
Results for commit 8bbf8b5 ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! The clearer messaging and separation in the test output is much nicer :) Thanks @pawelru!!
hi @khatril , please have a look what pawel wrote, and overall objective for this pr. :) |
This PR awaits updates of docker image - we need to have |
OK so this is green and approved so I can merge it but I think it would be better to merge #202 first and then I will update affected snapshots in this PR. Therefore, I kindly ask you to make a review of aforementioned PR :) |
Fundamental changes:
-as-cran
to enable snapshot testing. Currently, we render the whole book during the testing setup but we don't actually run the snapshot tests. This is a buggy behaviour that needs to be fixed.QUARTO_PROFILE
env var in the workflow definition. This will be then picked up by Quarto itself.stable
anddevel
profiles. Luckily, git recognizes changes as file rename (instead of remove & add).{variant}/path/{id}.md
but not possible :( I ran this using{path}_{variant}/snaps.md
but I am happy to change it to something else.Enhancements:
{tables/listings/graphs} / {subcategory}
, e.g.tables/ADA/adat01.qmd
instead ofadat.qmd
) in the.Rds
file path (created by knitr hooks insidetestthat/_data
) as well as snapshot file (variant
argument oftestthat::test_snapshot()
inside_snaps
directory). Previously it was based on the basename of an article only (e.g.adat01
) which is not super robust - there might be articles of the same basename in a separate directories.package/tests/testthat/test-snaps.R
file. I changed this to a simple for loop through the articles. I think it's easier to follow and it saves from redundant steps of e.g. removing ".rds" parts and adding this back soon after.test-snaps.R
file assuming future work for graphs type of tests.Other comments / thoughts - just for a record:
It's a little bit more complicated from the perspective of reverse-dependency-checks from within e.g. tern. There, we need to limit only to development profile most likely. But I left this problem for the future
test-snaps.R
that does for loop for each article. This implies that the snapshot file namesnaps.md
- this is impossible to change right now (I have reporter this feature request separately). We enhanced this a little usingvariant
argument so that it is{variant}/snaps.md
. An alternative approach would be to have a separate file for each article. This would create snapshot file in accordance to the name of the testing file. Together withvariant
could have{stable/devel}/table_ada/adat01.md
. This would be much more convenient for browsing as well as this would follow recommendations.I have been thinking about this and decided to keep it as is. The main rationale would be to make it super easy to include article in the regression testing. Currently it's enough to change the article code. The recommended way would also include additional step of adding a new test file - something that I would like to avoid.
On the other hand, the structure is pretty much stable and I personally don't foresee any significant changes. Therefore I think it should be ok to have separate test file for each article. Also, it's not a big deal if it will be missing - no test file means no test so nothing will fail. If done properly, this could also unblock
devtools::test(filter = "adat01")
type of things - however I am not sure if devs are using this. I'm interested in hearing your thoughts on this.