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
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions .github/workflows/test_notebooks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,19 @@ on:
push:
branches: [ develop, stable, nbtests ]
paths-ignore:
- '**/*.md' # ignore markdown files
- '**/*.rst' # ignore restructured text files
- '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_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.


Expand Down
Loading