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

End-to-End Tests on Generated Pipelines using Puppeteer #580

Merged
merged 48 commits into from
May 14, 2024

Conversation

garrettmflynn
Copy link
Member

@garrettmflynn garrettmflynn commented Feb 1, 2024

This PR will implement an end-to-end testing suite that runs through generated and manual pipelines using Puppeteer.

@CodyCBakerPhD Before we can push forward on this reliably, we have to get the GIN testing data downloaded in the Github Actions workflow. I noticed how you're doing this in NeuroConv, but it wasn't clear how to specify where the datasets are downloaded to. For this case, I've made it an assumption that the E2E tests must have the GIN data at ~/NWB_GUIDE/test-data, as it isn't clear how to pass this path dynamically using vitest.

Can you clarify how we'd install the GIN data at this path for Github Actions to use?

@garrettmflynn garrettmflynn self-assigned this Feb 1, 2024
@CodyCBakerPhD
Copy link
Collaborator

CodyCBakerPhD commented Feb 1, 2024

Can you clarify how we'd install the GIN data at this path for Github Actions to use?

Most reliable way would probably be to cd to the path you want to save them for vitetest to find them (by running aws s3 cp --recursive ${{ secrets.S3_GIN_BUCKET }}/behavior_testing_data ./behavior_testing_data since . is current location; you can otherwise try to alter the second path of the CLI but AWS CLI in general can be finicky so this might be more reliable)

Though, is this just for the tutorial data? Did you want to add cases from the general data based testing suite?

If tutorial only, might be worth finishing up #530 to simplify the issue altogether

@garrettmflynn
Copy link
Member Author

You mean this PR? #530

@garrettmflynn
Copy link
Member Author

Either one is fine. Depends on how comprehensive we want to be (e.g. we could quite easily run through all the YAML pipelines automatically).

It would be great if all the tests worked for any user without any configuration (i.e. we have the data included somehow), but there are ways to seamlessly transition between the two states.

@CodyCBakerPhD
Copy link
Collaborator

Ah, yes, #530

@CodyCBakerPhD
Copy link
Collaborator

It would be great if all the tests worked for any user without any configuration (i.e. we have the data included somehow)

That's pretty tough to do in a way that scales well (even in actions we utilize caching) and is really interoperable

Simplest approach is for us to require the data to exist, and simplest approach for data to exist is to use tools specifically built for data transfer

@garrettmflynn
Copy link
Member Author

garrettmflynn commented Feb 1, 2024

#530 still requires that the testing data is on the user's computer, so we'll still have to install on the CI in a default location.

You're right, though, we will want users to be interacting with a multi-subject, multi-session dataset for the tutorial—so it would be nice to have this merged. I'll use the generated dataset for our manual pipeline test.

@garrettmflynn
Copy link
Member Author

That's pretty tough to do in a way that scales well (even in actions we utilize caching) and is really interoperable

Got it, thank you for the clarification. I'll use the approach you've defined above.

@CodyCBakerPhD
Copy link
Collaborator

still requires that the testing data is on the user's computer

What do you mean by 'testing data'? That PR does not require the GIN example data, it generates synthetic files, if those are the ones you're talking about. But since we're the ones making them we have full control where they get saved

@garrettmflynn
Copy link
Member Author

garrettmflynn commented Feb 1, 2024

Oh I see, I read your changes incorrectly. base_path is for where the generated data will be output.

Do you want to run E2E tests using the testing data at all, or d'you think that generating these files is sufficient—at least for now?

@garrettmflynn garrettmflynn changed the title End-to-End Tests (manual + generated pipelines) using Puppeteer End-to-End Tests on Generated Pipelines using Puppeteer Feb 1, 2024
@CodyCBakerPhD
Copy link
Collaborator

CodyCBakerPhD commented Feb 1, 2024

Do you want to run E2E tests using the testing data at all, or d'you think that generating these files is sufficient—at least for now?

I'll continue to think about it

Since this mainly came up as a way to automatically keep tutorial screenshots up to date, I think focusing on that first and leaving open the possibility of full real data-based testing suite going into the future might just be the best way to go

@garrettmflynn
Copy link
Member Author

Got it. Changed the name of this PR and will keep it in draft mode.

I'll work on #530 until merged, then continue to build out #582.

@garrettmflynn garrettmflynn changed the base branch from main to tutorial-pipeline-puppeteer February 1, 2024 20:38
Base automatically changed from tutorial-pipeline-puppeteer to main February 12, 2024 18:31
@CodyCBakerPhD
Copy link
Collaborator

@garrettmflynn Is coverage possible to track via e2e tests? That could reduce amount of other tests we need to add to increase it

@garrettmflynn
Copy link
Member Author

Hmm it looks possible but could be somewhat complicated. Having some issues figuring it out because of blocked tests after adding the DANDI_CACHE environment variable—but I'll try a few things and get back to you once we figure that out.

@CodyCBakerPhD
Copy link
Collaborator

@garrettmflynn Has this been replaced?

@garrettmflynn
Copy link
Member Author

Technically no—though we have pretty much decided not to pursue this.

This particular PR prepares the E2E tests to run through all of the generated test pipelines. Something outside the scope of the tutorials, but still possibly useful.

Do we want to approach this or just close this out?

@CodyCBakerPhD
Copy link
Collaborator

@garrettmflynn CI does not like something in the dependencies ATM - on any platform it seems

@garrettmflynn
Copy link
Member Author

garrettmflynn commented May 13, 2024

Thanks for flagging! Should be fixed.

Also was able to knock out the TDT issue here. Turns out it was more related to these changes than I thought

@CodyCBakerPhD
Copy link
Collaborator

Let me know when all CI are passing

Also looks like docs have a build issue too

Are all the tutorial changes intended in this PR? I had thought they were from a different branch

Base automatically changed from allow-only-sorting to main May 13, 2024 20:50
@CodyCBakerPhD
Copy link
Collaborator

Looked over everything, just one small question about the form of an error message and the rest looks good to go!

@CodyCBakerPhD CodyCBakerPhD enabled auto-merge May 14, 2024 15:14
@CodyCBakerPhD
Copy link
Collaborator

@garrettmflynn Thanks for the hard work on this, this is gonna be great for maintainability!

@CodyCBakerPhD CodyCBakerPhD merged commit 20cd235 into main May 14, 2024
21 of 22 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the full-pipeline-puppeteer branch May 14, 2024 15:20
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