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

Run shinytests2 only if the modules are affected #273

Open
wants to merge 52 commits into
base: main
Choose a base branch
from

Conversation

llrs-roche
Copy link

@llrs-roche llrs-roche commented Jan 28, 2025

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
Copy link
Author

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 🤔

@llrs-roche llrs-roche marked this pull request as ready for review January 31, 2025 11:42
@llrs-roche llrs-roche requested a review from a team as a code owner January 31, 2025 11:42
@llrs-roche
Copy link
Author

llrs-roche commented Feb 6, 2025

Lately I'm having some problems with the github action. The script pass bash linters but it failswhen I test it teal.modules.clinical. The problem is that it doesn't provide an informative error. Do you @cicdguy know what could be causing this? See this action failing from insightsengineering/teal.modules.clinical#1329

Details about the testing PR

I need to update the SHA commit each time I update this branch because of problem with @ on the branch name: https://github.com/orgs/community/discussions/150817

@walkowif
Copy link
Contributor

walkowif commented Feb 6, 2025

@llrs-roche Can you check whether setting the shell to bash for the step (like this) will help? After expanding the logs, it seems that your step runs in a default sh shell.

@llrs-roche
Copy link
Author

@walkowif It didn't help, https://github.com/insightsengineering/teal.modules.clinical/actions/runs/13177410818/job/36779894175. I tested the script with shell linters and there isn't an obvious error on the code.

@walkowif
Copy link
Contributor

walkowif commented Feb 6, 2025

I ran the code of your step here after adding two debug statements, and it seems that the reason for the failure is exit code 1 returned by git log -1 --pretty=%B | grep -F "[run-all-tests]".

@llrs-roche
Copy link
Author

@cicdguy Many thanks! I found the way to reproduce this locally and fix the issue.

@cicdguy
Copy link
Contributor

cicdguy commented Feb 6, 2025

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

@llrs-roche
Copy link
Author

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
Copy link
Contributor

walkowif commented Feb 7, 2025

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
Copy link
Author

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
Copy link
Contributor

cicdguy commented Feb 7, 2025

@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.

@@ -246,6 +246,14 @@ on:
required: false
type: boolean
default: false
fast-tests:
description: |
Should shinytests2 tests only run per modified teal module?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes. Please no.

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

@llrs-roche
Copy link
Author

@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
Copy link
Contributor

cicdguy commented Feb 7, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants