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

Download GIN Data for Pipeline Tests #795

Merged
merged 101 commits into from
May 30, 2024
Merged

Conversation

garrettmflynn
Copy link
Member

This PR fixes the issue noted in #792 where the pipeline tests don't actually test anything because there is no test data found.

@CodyCBakerPhD Would you mind adding all the appropriate secrets for this workflow to run?

@garrettmflynn garrettmflynn self-assigned this May 24, 2024
@CodyCBakerPhD
Copy link
Collaborator

The design of this has several issues (I know they are still present on NeuroConv side, that suite is a bit touchier to refactor) and this is an opportunity to at least start off on the right foot here

I highlighted the newer approach in this issue: hdmf-dev/hdmf#1113

I can handle this over the weekend unless you really want to take a crack at it

@CodyCBakerPhD
Copy link
Collaborator

CodyCBakerPhD commented May 27, 2024

@garrettmflynn OK I got this to a more streamlined point in terms of deployment, display, etc.

However, the pipeline tests are still skipping everything; perhaps a data path needs to be configured in some way? https://github.com/NeurodataWithoutBorders/nwb-guide/actions/runs/9258765691/job/25469483408?pr=795#step:17:209

Would it be possible to adjust those tests to fail instead of skip when files are missing (ideally with an informative error message of 'data not found' per test)?

Feel free to take over this PR again to try to get the CI working as expected

@garrettmflynn
Copy link
Member Author

Yeah the data needs to go to a very specific location in the filesystem. I've made it so that the test should fail if it isn't there (instead of skipping).

Comment on lines 36 to 43
- name: Set working directory as environment variable
run: echo "CURRENT_WORKING_DIR=$(pwd)" >> $GITHUB_ENV
if: runner.os != 'Windows'

- name: Set working directory as environment variable (Windows)
run: echo "CURRENT_WORKING_DIR=%cd%" >> $env:GITHUB_ENV
shell: cmd
if: runner.os == 'Windows'
Copy link
Collaborator

Choose a reason for hiding this comment

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

For this style, I would recommend this syntax at the top of the file

env:
  CURRENT_WORKING_DIR: ${{ github.workspace }}

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh sure. I'm hoping to remove this ASAP.

It shows that we can, in fact, save a .env file and display the value on Windows. The Set working directory as environment variable option was simply to see if we can access something like this cross-platform. Will try this global env variable out!

Just switched the test pipeline workflow to use the exact same configuration for saving the .env file. Should tell us whether we're configuring GitHub Actions wrong or the test pipeline action is somehow not compatible with saving an .env file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It shows that we can, in fact, save a .env file and display the value on Windows. The Set working directory as environment variable option was simply to see if we can access something like this cross-platform. Will try this global env variable out!

I saw that... CI can be really confusing sometimes

One thing it might be is the conda activation step, which swaps to a different shell type (and maybe the .env thing is only available from the default bash??)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep looks like it. Now that we're digging into this, it looks like the Live service tests are also not behaving as expected—possibly as a consequence of these inconsistencies.

In both Dev and Live tests, No DANDI API key provided is printed to the console—despite the latter doing a similar dump to an environment file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm although is this only happening in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nevermind, this was just a consequence of not registering the DANDI API Key secret

@CodyCBakerPhD
Copy link
Collaborator

CodyCBakerPhD commented May 30, 2024

OK just gonna get this in, continue work on Windows over on #816

@CodyCBakerPhD CodyCBakerPhD enabled auto-merge May 30, 2024 02:31
@CodyCBakerPhD CodyCBakerPhD merged commit 53584d0 into main May 30, 2024
20 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the download-gin-data-for-tests branch May 30, 2024 02:33
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