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

Exit notebook tests early on forks #1363

Merged
merged 6 commits into from
May 16, 2024
Merged

Conversation

frankharkins
Copy link
Member

@frankharkins frankharkins commented May 14, 2024

Suggestion on how to handle notebook testing on forks. Resolves #967.

This PR proposes always failing the test on forks, with advice to merge to another branch in this repo to test it before merging to the main branch. While a little clunky, we very rarely get PRs from forks so this is probably simplest for now. We can review the process if it starts becoming a problem.

@Qiskit Qiskit deleted a comment from review-notebook-app bot May 14, 2024
@frankharkins frankharkins marked this pull request as ready for review May 14, 2024 09:36
@Eric-Arellano
Copy link
Collaborator

Thanks for looking into this! I don't think the feature branch approach will work super well. People without write access cannot create the initial feature branch to merge into. We could set up the feature branch for them and always use the same one like fork-testing - if there are issues, then they would not be able to push updates to the fork-testing branch so they would need to instead open another PR to modify fork-testing. PR review is often very iterative, so that might be clunky.

Instead, if we wanted to support testing from a fork, I think we would need to use something nifty like https://github.com/imjohnbo/ok-to-test. That is a bit complex, though, and I'm skeptical it's worth the complexity because we so rarely get contributions from people without write access.

Still, this PR is really valuable to error on forks because:

  1. A lot of times people do have write access and don't realize they shouldn't be using a fork, since that goes against open source convention. It's good to course correct them.
  2. For people without write access, it's a signal to the PR reviewers to be careful that it has not been tested.

I'd recommend we take this approach for now - still error on forks, but reword the error message:

  • If you have write access, instead push to qiskit/documentation (upstream)
  • If you don't have write access, please test it out locally by following the README instructions. Maybe include a note for code reviewers about how they should approach the situation.

Another concern with erroring on forks is we won't execute the code that validates all notebooks are classified. Ideally we could validate that, even if we skip running the notebooks. The best idea I can think of is to add a CLI arg to the notebook test that will skip all execution and only validate the classification.

Wdyt?

@frankharkins
Copy link
Member Author

I think we had similar ideas; I've edited the message to be clearer. I have an idea about the classification validation; will make a follow-up PR.

Copy link
Collaborator

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Thanks!

.github/workflows/notebook-test.yml Outdated Show resolved Hide resolved
Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
@frankharkins frankharkins enabled auto-merge May 16, 2024 13:19
@frankharkins frankharkins added this pull request to the merge queue May 16, 2024
Merged via the queue into main with commit c6f6dc6 May 16, 2024
2 checks passed
@frankharkins frankharkins deleted the FH/disable-tests-on-forks branch May 16, 2024 13:23
frankharkins added a commit to frankharkins/documentation that referenced this pull request Jul 22, 2024
Suggestion on how to handle notebook testing on forks. Resolves Qiskit#967.

This PR proposes always failing the test on forks, with advice to merge
to another branch in this repo to test it before merging to the main
branch. While a little clunky, we very rarely get PRs from forks so this
is probably simplest for now. We can review the process if it starts
becoming a problem.

---------

Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Work out how to handle notebook testing on forks
2 participants