Run shinytests2 only if the modules are affected

[llrs-roche]

Pull Request

Fixes: https://github.com/insightsengineering/coredev-tasks/issues/580

This should make testing on PR to main a lot faster by just running test of the modules affected.

TODO:

• Check if modifying a general files trigger all tests
• Be sensible to the branch name and merging strategy: 125_feat@dev@main should test and compare vs @dev ?
• Check it works on a package without modules (teal.code for example)
• Check testing depth change doesn't affect other other steps.

[llrs-roche]

When a module file or a test file is modified this PR lowers the testing threshold environmental variable when the threshold of the test file is lowered too.

Any change on the R files that are not found on tests/testthat/test-shinytest2- will trigger the full check even if there is a file that modifies only a module: The package is fully checked if a helper function is modified.

I have created a local branch on tmc to test this, and also tested it on teal.code.

• Be sensible to the branch name and merging strategy: 125_feat@dev@main should test and compare vs @dev ?
  I couldn't find a way to do that in Github actions, I could check this inside the step but I think this is enough too much complication.

• Check testing depth change doesn't affect other other steps.
  I can push the tmc branch mentioned above with a yaml file that points to this branch. Maybe there is a different testing method 🤔

[cicdguy]

@walkowif is the guy to thank, I didn't do anything haha

[llrs-roche]

Thanks @walkowif and @cicdguy for your help, I'd appreciate further help to finish this PR. I don't think I can't take this further and it would be a really great feature to have before tmg and tmc releases.

The key points of the new step are described on a comment: we want to make checks faster by only testing what have been modified. I think the bash script logic is correct, but I cannot test if it works well on the testing branch for tmc. There are differences between the local repository and the repository created by the github action. These differences make it difficult for me to resolve them.

Could you modify this branch so that the testing branch reaches the end of the small bash script?
Current problems arise from modifications to the files on the repository that prevent comparing it with the main branch or having conflicts that prevent it.
Thanks in advance

[walkowif]

Maybe you could try using this GH Action instead to get the files modified in a given PR? The example in the readme shows how to loop over the changed files so that you could process them according to the logic you already have in your script.

[llrs-roche]

Thanks for the suggestion, using that external action is in my opinion risky: It is updated quite often and it is not clear to me what does on each action (the behavior seems a bit different between main and a PR). In addition I don't know how well it will work with the changes on the repository done by the action itself. I am testing it but is up to you.

[cicdguy]

@llrs-roche - yes there is a risk with using external actions. The same applies to using anything in the open source community. We already this and many other external actions in our workflows so it's okay for us to assume the risk. Also, actions are versioned so make sure you pin the version when you use it.

[cicdguy]

Yikes. Please no.

This package template's workflows are used for more than just teal. Please don't make this teal-specific.

[llrs-roche]

The step is only mean to affect packages defining teal modules and that use a specific function used by teal.modules.clinical and teal.modules.general (skip_if_too_deep). Currently on the CI runs for 1 hour to test all the modules on teal.modules.clinical and we wish to have faster feedback from the CI. This PR would help specially when we only modify a single module.

What would be the best approach here? Should we remove the mention to teal, move this step to a specific file only meant for teal packages or something else?

[cicdguy]

If that is the case, I'd recommend using the setup() and teardown() functionalities in testthat() to achieve the same results.

Remember - this package template is also a package template for the OSS community. It also serves 90 other repositories: https://github.com/insightsengineering/r.pkg.template/network/dependents

[llrs-roche]

From setup and teardown functionalities:

We no longer recommend using setup() and teardown(); instead we think it's better practice to use a test fixture as described in vignette("test-fixtures").

We do not only setup environmental variables but also need to modify a variable used as input parameter inside the tests. While we could setup fixture to address the first step, we can't do the later with fixtures.

[m7pr]

@cicdguy and @llrs-roche I think this improvement is fine. Just the wording is improper.
Do not talk about teal, nor tmg nor tmc.
Just think and write about packages with shinytest2 tests.

Rename

      fast-tests:
        description: |
          Should shinytests2 tests only run per modified teal module?

to

      selected-shinytests:
        description: |
          Should shinytests2 tests only run per modified corresponding R file in R/ folder?

and make this functionality robust and flexible. So it does not assume the input package is tmg or tmc, but any R package that has scripts in R and tests in shinytest. The only assumption we can make is that shinytest inherit names after scripts in R folder.

[m7pr]

On another hand we can only introduce changes in workflows in tmc repository. We don't need to edit the whole template used in all repositories. However I see benefits that other packages could utilize this functionality.

[cicdguy]

Yes if we were to rephrase it according to this suggestion, that should be fine.

[llrs-roche]

@cicdguy and @m7pr thanks for the feedback, I'll modify the variable and the description accordingly. Should I mention the specific function being modified on tests skip_if_too_deep(5) -> skip_if_too_deep(3)?

[cicdguy]

Yes best to mention what is being modified. Also add a note saying that it might not apply to most packages.

[llrs-roche]

@cicdguy The problem is not with using an open source action but on how stable it is. If it needs many updates are you okay with using it with security problems? Or updating every X months?
In addition it requires further changes on the pipeline: https://github.com/insightsengineering/teal.modules.clinical/actions/runs/13200738787/job/36851913660

[cicdguy]

The problem is not with using an open source action but on how stable it is

Yes, and that's why I'm asking you to pin the version. If the version is pinned, you're going to end up using that specific version, right? Then the risk of encountering an API change will be minimal.

If it needs many updates are you okay with using it with security problems? Or updating every X months?

Yes, we do this anyway.

[llrs-roche]

Many thanks all for your feedback. This branch is ready for another round of reviews.
Last time, there was a request to verify that it worked on three conditions (and modify the documentation):

• scenario where only utils function got changed
  PR on tmc, action/job
• scenario where only modules file got changed
  PR on tmc, action/job
• scenario where only test file got changed
  PR on tmc, action/job

I'd appreciate your feedback @m7pr and @cicdguy. Some questions I would make or comment as a reviewer.

• Are the messages of the new steps informative?
  Maybe I should provide more information or word the feedback differently.
• Is the description/documentation general enough?
• Is the bash code of the action readable? Or could it be improved?
• With the same sha on all actions, did it made testing run faster (acceptance criteria)?
  The utils branch should take longer (>1.3h), then the module branch (should that at most as the full check) and last the test branch (~20 min).
• Are there any side effects?
  I noticed that non-CRAN ran faster than CRAN checks. I'm not sure what causes this