Skip to content

Remove fixed Ubuntu 20.04 testing workflow#2397

Merged
adamrtalbot merged 3 commits into
nf-core:devfrom
adamrtalbot:use_one_testing_workflow
Aug 15, 2023
Merged

Remove fixed Ubuntu 20.04 testing workflow#2397
adamrtalbot merged 3 commits into
nf-core:devfrom
adamrtalbot:use_one_testing_workflow

Conversation

@adamrtalbot

Copy link
Copy Markdown
Contributor

Removes the pytest fixed version on Ubuntu:20.04 and Python 3.8 and adds it to matrix variables.

Original merge that added it: #2043

Using fewer workflows it better for enforcing code coverage etc. and managing testing throughput.

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!
  • Documentation in docs is updated

@github-actions

Copy link
Copy Markdown
Contributor

This PR is against the master branch ❌

  • Do not close this PR
  • Click Edit and change the base to dev
  • This CI test will remain failed until you push a new commit

Hi @adamrtalbot,

It looks like this pull-request is has been made against the adamrtalbot/tools master branch.
The master branch on nf-core repositories should always contain code from the latest release.
Because of this, PRs to master are only allowed if they come from the adamrtalbot/tools dev branch.

You do not need to close this PR, you can change the target branch to dev by clicking the "Edit" button at the top of this page.
Note that even after this, the test will continue to show as failing until you push a new commit.

Thanks again for your contribution!

@adamrtalbot adamrtalbot changed the title Use one testing workflow Remove fixed Ubuntu 20.04 testing workflow Aug 14, 2023
@adamrtalbot adamrtalbot changed the base branch from master to dev August 14, 2023 15:15
@codecov

codecov Bot commented Aug 14, 2023

Copy link
Copy Markdown

Codecov Report

Merging #2397 (38f986f) into dev (3ec2697) will increase coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##              dev    #2397      +/-   ##
==========================================
+ Coverage   71.09%   71.14%   +0.05%     
==========================================
  Files          87       87              
  Lines        9409     9409              
==========================================
+ Hits         6689     6694       +5     
+ Misses       2720     2715       -5     

see 1 file with indirect coverage changes

@edmundmiller edmundmiller left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me, I didn't know you could do that!

Comment on lines +17 to +18
env:
GITHUB_TOKEN: ${{ github.token }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
env:
GITHUB_TOKEN: ${{ github.token }}

What's the purpose of adding this? Just wondering for our future selves! 😄

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I was hitting API limits, I hoped it might authenticate by default and it seemed to help! Looking at the code I think it's just coincidence but maybe it worked.

I bet if I remove it it will break.

pip install -e .

- name: Downgrade git to the Ubuntu official repository's version
if: ${{ matrix.runner == 'ubuntu-20.04' && matrix.python-version == '3.8' }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
if: ${{ matrix.runner == 'ubuntu-20.04' && matrix.python-version == '3.8' }}
if: ${{ matrix.runner == 'ubuntu-20.04' && matrix.python-version == '3.8' }}
env:
GITHUB_TOKEN: ${{ github.token }}

Is this the scope it's needed in?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yep, it reflects the original test which used a couple of older versions of software together to mimic a scenario. If you look at the tests it seems to be working correctly.

@adamrtalbot adamrtalbot merged commit 9d63f93 into nf-core:dev Aug 15, 2023
@adamrtalbot adamrtalbot deleted the use_one_testing_workflow branch August 15, 2023 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants