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

tests: Add test case for cross-organization subwf #6

Merged
merged 5 commits into from
Jul 8, 2024
Merged

Conversation

jvfe
Copy link
Collaborator

@jvfe jvfe commented Jun 21, 2024

Strangely it is passing at the moment, it seems the install command returns a passing exit code regardless of installing all modules.

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

Strangely it is passing at the moment, it seems the install command
returns a passing exit code regardless of installing all modules.

- Addresses #2
@jvfe jvfe marked this pull request as draft June 21, 2024 18:46
@jvfe jvfe linked an issue Jun 21, 2024 that may be closed by this pull request
@jvfe jvfe added this to the Reproducible error example milestone Jun 21, 2024
@jvfe jvfe added the testing/linting Issue related to the tests or the linting functions label Jun 21, 2024
@muffato
Copy link
Member

muffato commented Jun 25, 2024

You mean it's passing locally ? I can't see any action run here on GitHub CI

@muffato
Copy link
Member

muffato commented Jun 25, 2024

Ah, that's because Actions are turned off by default on forked repositories.
Screenshot 2024-06-25 at 11 42 37

I've now enabled them, but probably they will only kick in next time there's a commit on this branch.

@jvfe
Copy link
Collaborator Author

jvfe commented Jun 25, 2024

Ah, that's because Actions are turned off by default on forked repositories.

Ooh ok that makes sense, I had tested it locally.
Either way, I just manually submitted the action and it does show what I said: https://github.com/sanger-tol/nf-core-tools/actions/runs/9661393766/job/26649018814
Another test failed (not related to my changes, it's just changelog/bump issues), but mine succeeds.

I'll test the actual subworkflow install command manually (outside of the test) and capture the exit code, but that seems like it's returning a 0 even when it doesn't install every component.

@jvfe
Copy link
Collaborator Author

jvfe commented Jun 26, 2024

Yep, that's definitely the case, when running:

nf-core subworkflows --git-remote https://github.com/jvfe/test-subworkflow-remote install hic_bwamem2

and then getting the exit code:

echo $?

it returns 0.

@jvfe
Copy link
Collaborator Author

jvfe commented Jun 28, 2024

Yep, that's definitely the case, when running:

nf-core subworkflows --git-remote https://github.com/jvfe/test-subworkflow-remote install hic_bwamem2

and then getting the exit code:

echo $?

it returns 0.

Apparently this is expected (discussed here). So the way to go is to check for the missing module in the module json list, which is what 568363a does.

@jvfe
Copy link
Collaborator Author

jvfe commented Jun 28, 2024

Yep, now it's failing as expected: https://github.com/sanger-tol/nf-core-tools/actions/runs/9717099844/job/26822060543
For some reason the actions are not running automatically, I'll check this later.

@jvfe jvfe marked this pull request as ready for review June 28, 2024 20:43
@muffato
Copy link
Member

muffato commented Jul 1, 2024

Looks fine.

Yep, now it's failing as expected: https://github.com/sanger-tol/nf-core-tools/actions/runs/9717099844/job/26822060543 For some reason the actions are not running automatically, I'll check this later.

Yep, I'm wondering too !

Also, if the return value of install(...) is not an indicator of success/failure, why is it checked in other parts of tests/subworkflows/install.py ? If something's indeed not right, make it an issue on the nf-core repo.

@jvfe jvfe changed the base branch from fix/1927 to master July 1, 2024 11:34
@jvfe jvfe changed the base branch from master to fix/1927 July 1, 2024 11:42
@jvfe
Copy link
Collaborator Author

jvfe commented Jul 1, 2024

Looks fine.

Yep, now it's failing as expected: https://github.com/sanger-tol/nf-core-tools/actions/runs/9717099844/job/26822060543 For some reason the actions are not running automatically, I'll check this later.

Yep, I'm wondering too !

Also, if the return value of install(...) is not an indicator of success/failure, why is it checked in other parts of tests/subworkflows/install.py ? If something's indeed not right, make it an issue on the nf-core repo.

Thanks! I've now created the issue. I was hesitant to create it at first since given the conversation on slack I thought it was something expected.

Yeah and I'm not sure why the CI wasn't running, but I just sent some dummy commits and now it runs. I tried to change the base branch for this PR to see if that was the reason behind it (like if maybe it just runs the CI for PRs against the main branch), but apparently not.

@jvfe
Copy link
Collaborator Author

jvfe commented Jul 8, 2024

Apparently the issue with the modules test failing is unrelated to my changes, because, after some local testing, I could see it also fails in the master branch. Given that's the case, I'll be merging this then.

@jvfe jvfe merged commit 6816cc1 into fix/1927 Jul 8, 2024
31 of 34 checks passed
jvfe pushed a commit that referenced this pull request Aug 5, 2024
@jvfe jvfe deleted the cross-org-test branch August 13, 2024 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing/linting Issue related to the tests or the linting functions
Development

Successfully merging this pull request may close these issues.

Add test case for subworkflow with different module remotes
2 participants