Run tests with a realistic version of git #2043

Aratz · 2022-11-22T14:53:45Z

This PR fixes the following issues:

Github's Ubuntu images use the very latest version of git (i.e. v2.38.1, see here). More specifically, this is different from the version available on the image that is used for the tests (Ubuntu 20.04, git 2.25.1). Since nf-core/tools heavily depends on the git version available to the user, this PR downgrades git so that the testing environment is more similar to what a typical user would have.
Consequently, some failed tests have been going under the radar in test_create and test_sync. This PR also fixes these tests.

Additionally, this is at least the second time we encounter bugs that are specific to the version of git being used so maybe we should consider testing with older and newer LTS Ubuntu versions (and their respective versions of git)?

PR checklist

This comment contains a description of changes (with reason)
CHANGELOG.md is updated
If you've fixed a bug or added code that should be tested, add tests!

codecov · 2022-11-22T15:04:10Z

Codecov Report

Merging #2043 (452f243) into dev (ea388b3) will decrease coverage by 0.14%.
The diff coverage is n/a.

❗ Current head 452f243 differs from pull request most recent head 755b9f2. Consider uploading reports for the commit 755b9f2 to get more accurate results

@@            Coverage Diff             @@
##              dev    #2043      +/-   ##
==========================================
- Coverage   67.76%   67.61%   -0.15%     
==========================================
  Files          43       43              
  Lines        5559     5558       -1     
==========================================
- Hits         3767     3758       -9     
- Misses       1792     1800       +8

Impacted Files	Coverage Δ
nf_core/components/components_utils.py	`32.20% <0.00%> (-15.99%)`	⬇️
nf_core/lint_utils.py	`73.68% <0.00%> (-12.04%)`	⬇️
nf_core/modules/modules_utils.py	`62.79% <0.00%> (-11.02%)`	⬇️
nf_core/components/components_test.py	`56.09% <0.00%> (-2.14%)`	⬇️
nf_core/components/info.py	`78.72% <0.00%> (-1.28%)`	⬇️
nf_core/modules/test_yml_builder.py	`49.09% <0.00%> (-0.46%)`	⬇️
nf_core/__main__.py	`58.62% <0.00%> (+0.16%)`	⬆️
nf_core/components/update.py	`82.21% <0.00%> (+0.49%)`	⬆️
nf_core/modules/lint/__init__.py	`83.42% <0.00%> (+0.53%)`	⬆️
nf_core/modules/modules_repo.py	`84.13% <0.00%> (+0.64%)`	⬆️
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

fabianegli

I think this makes a lot of sense, but we could also just alert people who have an outdated version of git installed to please update. That might be the simpler solution.

The git customization adds 20+ seconds to the test setup - We might want to somehow set a sunset trigger for this to remove it once newer versions of git can be reasonably expected. This could be in an issue, or as a trigger in the setup that fails when the installed git version after the setup is newer than some version.

fabianegli · 2022-11-22T20:12:04Z

.github/workflows/pytest.yml

+ sudo apt remove git git-man
+ sudo add-apt-repository --remove ppa:git-core/ppa
+ sudo apt install git


I am not knowledgable enough to judge this, but isn't there an apt update missing before the installation? Or is it intentionally missing?

apt update is needed when you add a new source and need to update package information. When you remove a source, there is no new info to get :)

I meant between the repository removal and the install. I figured running update after removing the repository might change what package install pulls.

mashehu · 2022-11-22T20:39:26Z

That's a great catch, thank you!

Aratz · 2022-11-23T07:36:53Z

Thanks for the quick replies!

@fabianegli I think few people are likely to add their own PPA to get the latest version and will just go with the latest version available in the official Ubuntu repository (2.17 for Ubuntu 18.04 and 2.25 for Ubuntu 20.04). Ubuntu 20.04 will be supported until 2025-04 and I think we should do just the same. Some people just don't have the option to update the system they are given access to.

I think in general it makes sense to use the git package from the official Ubuntu repository rather than from the custom PPA, so I'd rather keep this even in the future. I think that, as much as possible, the test environment should look like what users will have. I mean the bugs we found were due to the new default branch feature but it could be something else with future versions of git.

fabianegli · 2022-11-23T09:38:40Z

@Aratz I agree with you for basically everything.

fabianegli · 2022-11-24T10:27:24Z

Related, but a separate issue: #2044

Aratz · 2022-11-25T10:28:08Z

I think I've managed to fix it. I've creating a release on my own fork for validation purposes. The tests fail for unrelated reasons but the full matrix was scheduled at least: https://github.com/Aratz/nf-core-tools/actions/runs/3547118628/jobs/5956896579#step:7:81

fabianegli · 2022-11-25T10:41:48Z

It would be nice to have a way to query our past runs to see how often specific python versions had failing tests while others succeeded to get some data on wether dropping all in-between version tests for both ubuntu version is reasonable. Otherwise I would try to keep testing all Python versions on the newer Ubuntu release.

mashehu · 2022-11-25T10:43:27Z

.github/workflows/pytest.yml

+ steps:
+ - id: set-matrix
+ run: |
+ if [ ${{ github.event_name }} == "release" ]; then


If we run this only for PRs towards master (which is basically a release) the logic might be bit simpler no?

I'm not sure I follow, do you mean the select-strategy job should run only on PR that target master? I think this becomes a problem in the pytest job because of the needs: select-strategy line, i.e. the way it is written it is expected select-strategy is run in all cases.

I've tried to implement this using only "workflow logic" but couldn't really come up with a good solution, so that's why I had to incorporate these bash lines instead.

Aratz · 2022-11-25T14:57:00Z

@fabianegli hmm, yes in that case the workflow description would be much simpler actually. Maybe testing every python version on ubuntu-22.04 and only python-3.8 on ubuntu-20.04 (that is, the version available on the official repository) is enough?

fabianegli · 2022-11-27T11:40:21Z

@Aratz yes, I think this would suffice. We'd then have the tests for the "frozen system" Ubuntu 20.04 with it's original python and git version plus a more comprehensive approach regarding Python versions on "ubuntu-latest". And now I am thinking that it might be preferable to set the test up in a separate workflow named something like "pytest-frozen-ubuntu-20.04" and keep the current pytest workflow as is - trading some code duplication for easier communication of intent.

The issue here is that renaming a branch in an empty repository will fail with earlier versions of git (e.g. 2.25), where the default branch is always master anyways.

fabianegli

I think this does what we want/need. Well done @Aratz

Aratz requested review from mashehu and fabianegli November 22, 2022 14:53

Aratz self-assigned this Nov 22, 2022

Aratz force-pushed the fix_git_rename branch from a890a63 to ffdae0d Compare November 22, 2022 14:55

fabianegli reviewed Nov 22, 2022

View reviewed changes

fabianegli mentioned this pull request Nov 24, 2022

ubuntu-latest will soon point to the next major Ubuntu version #2044

Closed

mashehu reviewed Nov 25, 2022

View reviewed changes

Aratz added 3 commits November 30, 2022 13:30

Rename default branch after committing

1e8ccce

Only rename default branch if it is not master

f35fbfb

The issue here is that renaming a branch in an empty repository will fail with earlier versions of git (e.g. 2.25), where the default branch is always master anyways.

Update changelog

6d150ff

Aratz force-pushed the fix_git_rename branch 2 times, most recently from a257e35 to 755b9f2 Compare November 30, 2022 12:33

Freeze tests for Python 3.8/Ubuntu 20.04

755b9f2

fabianegli approved these changes Nov 30, 2022

View reviewed changes

Aratz merged commit 07f64b9 into nf-core:dev Nov 30, 2022

adamrtalbot mentioned this pull request Aug 14, 2023

Remove fixed Ubuntu 20.04 testing workflow #2397

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run tests with a realistic version of git #2043

Run tests with a realistic version of git #2043

Aratz commented Nov 22, 2022 •

edited

Loading

codecov bot commented Nov 22, 2022 •

edited

Loading

fabianegli left a comment

fabianegli Nov 22, 2022

Aratz Nov 23, 2022

fabianegli Nov 23, 2022

mashehu commented Nov 22, 2022

Aratz commented Nov 23, 2022

fabianegli commented Nov 23, 2022 •

edited

Loading

fabianegli commented Nov 24, 2022

Aratz commented Nov 25, 2022

fabianegli commented Nov 25, 2022

mashehu Nov 25, 2022

Aratz Nov 25, 2022

Aratz commented Nov 25, 2022

fabianegli commented Nov 27, 2022

fabianegli left a comment

Run tests with a realistic version of git #2043

Run tests with a realistic version of git #2043

Conversation

Aratz commented Nov 22, 2022 • edited Loading

PR checklist

codecov bot commented Nov 22, 2022 • edited Loading

Codecov Report

fabianegli left a comment

Choose a reason for hiding this comment

fabianegli Nov 22, 2022

Choose a reason for hiding this comment

Aratz Nov 23, 2022

Choose a reason for hiding this comment

fabianegli Nov 23, 2022

Choose a reason for hiding this comment

mashehu commented Nov 22, 2022

Aratz commented Nov 23, 2022

fabianegli commented Nov 23, 2022 • edited Loading

fabianegli commented Nov 24, 2022

Aratz commented Nov 25, 2022

fabianegli commented Nov 25, 2022

mashehu Nov 25, 2022

Choose a reason for hiding this comment

Aratz Nov 25, 2022

Choose a reason for hiding this comment

Aratz commented Nov 25, 2022

fabianegli commented Nov 27, 2022

fabianegli left a comment

Choose a reason for hiding this comment

Aratz commented Nov 22, 2022 •

edited

Loading

codecov bot commented Nov 22, 2022 •

edited

Loading

fabianegli commented Nov 23, 2022 •

edited

Loading