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

Update tests to ignore Markdown, RST and template and landing page notebooks #1194

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

robbibt
Copy link
Member

@robbibt robbibt commented Feb 27, 2024

Proposed changes

This PR updates the "ignore-paths" line of our Github Actions testing workflow to ignore future updates to the repo's Readme, template notebook or landing page notebook.

This will prevent us having to wait ~30 minutes for tests to run after making minor updates to Markdown or RST files.

@robbibt robbibt marked this pull request as ready for review February 27, 2024 02:38
@robbibt robbibt changed the title Update ignored paths for tests Update tests to ignore Markdown, RST and template and landing page notebooks Feb 27, 2024
- 'DEA_Sandbox.ipynb' # ignore landing page
- 'DEA_notebooks_template.ipynb' # ignore template
- '*.md' # ignore all markdown files
- '*.rst' # ignore all restructured text files
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that this **/* syntax is needed to ignore all of them recursively, not just in the root directory.

      - '**/*.md' # ignore markdown files
      - '**/*.rst' # ignore restructured text files

Copy link
Member Author

@robbibt robbibt Feb 27, 2024

Choose a reason for hiding this comment

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

I don't think the recursive ignore was ignoring the top level markdown files in your previous PR - it seemed to be only catching the ones nested within subfolders.

We could leave the recursive ignore as it is though and adding in your suggested top level "*" ignore to catch the Readme?

Copy link
Member Author

@robbibt robbibt Feb 27, 2024

Choose a reason for hiding this comment

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

Actually, maybe it just needs to be "**.md?" to catch both top level and nested files?
https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#patterns-to-match-file-paths

- '.github/**' # ignore anything in .github folder
- '!.github/workflows/test_notebooks.yml' # except test_notebooks.yml
pull_request:
branches: [ develop, stable ]
paths-ignore:
- '**/*.md'
- '**/*.rst'
- 'DEA_Sandbox.ipynb'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you sure this should be ignored? It is a Notebook file that we need to check renders properly on the Sandbox.

Copy link
Member Author

@robbibt robbibt Feb 27, 2024

Choose a reason for hiding this comment

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

It doesn't actually include any code - it's all entirely pre-rendered markdown cells so there's not anything for pytest to run to test for a pass/fail.

- 'DEA_Sandbox.ipynb'
- 'DEA_notebooks_template.ipynb'
- '*.md'
- '*.rst'
- '.github/**'
- '!.github/workflows/test_notebooks.yml'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should ignore everything in the root directory except for Notebook files:

      - '*'
      - '!*.ipynb'

Copy link
Member Author

@robbibt robbibt Feb 27, 2024

Choose a reason for hiding this comment

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

I'd be happy to ignore all top-level files - the two notebooks there are either entirely Markdown or are not intended to be run by our users (both aren't included in our test coverage anyway, and DEA_notebooks_template.ipynb isn't even synced to the Sandbox).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree. Sounds good.

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.

3 participants