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

Add CI for testing pyflask distributable #297

Merged
merged 18 commits into from
Aug 18, 2023

Conversation

CodyCBakerPhD
Copy link
Collaborator

Re-attempt #292 from a better base

@CodyCBakerPhD
Copy link
Collaborator Author

Unfortunately to see if the workflow works, it must be merged to main to trigger

@CodyCBakerPhD CodyCBakerPhD marked this pull request as ready for review August 17, 2023 03:33
@CodyCBakerPhD CodyCBakerPhD self-assigned this Aug 17, 2023
Comment on lines 89 to 96
run: node tests/testPyinstallerExecutable.js ./build/nwb-guide/nwb-guide.exe

- if: matrix.os != "windows-latest"
name: Run test on build executable
run: node tests/testPyinstallerExecutable.js ./build/nwb-guide/nwb-guide

- name: Run test on distributed executable
run: node tests/testPyinstallerExecutable.js
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There was some confusion on my part about this - after double checking the flags to pyinstaller, I see that the default path of your tests is indeed the 'distributable' (./build/flask/nwb-guide/nwb-guide.exe) and not the 'build' (./build/nwb-guide/nwb-guide.exe)

I've simply added the tests on the build, as PyInstaller refers to them, to be exhaustive in case there ever could be any difference between the two

Copy link
Member

@garrettmflynn garrettmflynn Aug 17, 2023

Choose a reason for hiding this comment

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

Oh sure. For some reason, I'd thought you meant that you'd target the dist/mac/NWB GUIDE.app/Contents/Resources/nwb-guide/nwb-guide file that is produced after copying the distributable file to the Electron app.

TBH I'm never touching the 'build' version, which is contained in a folder that primarily contains Pyinstaller logs, so we might just ignore it since it's unused.

@CodyCBakerPhD CodyCBakerPhD changed the title Create pyflask-build-and-dist-tests.yml Add CI for testing pyflask distributable Aug 17, 2023
Base automatically changed from simpler_flask_reduction to main August 18, 2023 02:41
@CodyCBakerPhD
Copy link
Collaborator Author

🎉Able to reproduce scipy issue in stable windows CI: https://github.com/NeurodataWithoutBorders/nwb-guide/actions/runs/5898618230/job/15999924669?pr=297

Mac indeed appears to be in some kind of infinite loop - your test should simply exit out if the server can launch successfully right? https://github.com/NeurodataWithoutBorders/nwb-guide/actions/runs/5898618230/job/15999924617?pr=297

@CodyCBakerPhD
Copy link
Collaborator Author

And then Linux appears to be missing the json schema paths: any idea how to fix? https://github.com/NeurodataWithoutBorders/nwb-guide/actions/runs/5898618230/job/15999924554?pr=297

@garrettmflynn
Copy link
Member

Mac indeed appears to be in some kind of infinite loop - your test should simply exit out if the server can launch successfully right? https://github.com/NeurodataWithoutBorders/nwb-guide/actions/runs/5898618230/job/15999924617?pr=297

Actually, I didn't configure this properly for a test. Currently, there are errors thrown if the process fails—though the process remains open infinitely (as Flask does) if successful.

I'm listening for startup time in the proc.stdout.on("data", () => {}) function.Just inserted a call to process.exit() so the main process closes gracefully. Hopefully this will work?

@garrettmflynn
Copy link
Member

And then Linux appears to be missing the json schema paths: any idea how to fix? https://github.com/NeurodataWithoutBorders/nwb-guide/actions/runs/5898618230/job/15999924554?pr=297

This is fixed in #284. We'd want to rebase to that branch if we want those changes here.

@CodyCBakerPhD
Copy link
Collaborator Author

This is fixed in #284. We'd want to rebase to that branch if we want those changes here.

In that case what I propose as the simplest approach is, despite the currently failures (they are not required to pass to merge) we go ahead and get this into main so every PR can benefit and test fixes specific to each branch

What do you think?

@garrettmflynn
Copy link
Member

garrettmflynn commented Aug 18, 2023

Yeah I agree. I'll definitely need to clean up the error catching (as the ones thrown by Mac aren't caught by my fix)—but at least this will let us test our current versions.

My local builds are stalling, otherwise I'd fix it now.

@CodyCBakerPhD
Copy link
Collaborator Author

My local builds are stalling, otherwise I'd fix it now.

Should we wait for those test function improvements before merging this then?

By stalling do you mean you're also seeing this infinite loop behavior? I think you mentioned it might have been because of those recent changes I made to the Flask version fetching so feel free to disable that if needed

@garrettmflynn
Copy link
Member

Just pushed a preliminary version of those so I don't lose the changes.

By stalling do you mean you're also seeing this infinite loop behavior? I think you mentioned it might have been because of those recent changes I made to the Flask version fetching so feel free to disable that if needed

No, currently the build will run without logs as long as I wait (~5min). For some reason, my local build is being finicky.

@CodyCBakerPhD
Copy link
Collaborator Author

No, currently the build will run without logs as long as I wait (~5min). For some reason, my local build is being finicky.

Does checking out a point on main from a couple days ago work as expected?

@garrettmflynn
Copy link
Member

Actually this is also happening with npm run dev:server. Something must be up...

@garrettmflynn
Copy link
Member

Will yours run on Mac? Trying to add logs to determine the point of failure but it's as if nothing is actually run.

@CodyCBakerPhD
Copy link
Collaborator Author

Will yours run on Mac? Trying to add logs to determine the point of failure but it's as if nothing is actually run.

Checking out and pulling latest main and calling npm start or npm run dev:servercurrently works fine on my M1

I run into these kinds of things from time to time though - if you wipe everything (including git clone) and hard reinstall everything (including the npm ci or npm install stuff) it might go back to normal - or if not at least that's informative

@garrettmflynn
Copy link
Member

Just restarted my computer and both work fine now.

@CodyCBakerPhD
Copy link
Collaborator Author

Good to hear - oh, hey, the mac test now fails with an actual errror! https://github.com/NeurodataWithoutBorders/nwb-guide/actions/runs/5904881769/job/16017892212?pr=297

@garrettmflynn
Copy link
Member

Alright should be good to merge. Tests failed but my new code works to close the server if an error / success is detected.

@CodyCBakerPhD
Copy link
Collaborator Author

@garrettmflynn Cool, just need approval

Copy link
Member

@garrettmflynn garrettmflynn left a comment

Choose a reason for hiding this comment

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

Approved! Let's see if this helps on the active branches.

@CodyCBakerPhD CodyCBakerPhD merged commit 172bbb7 into main Aug 18, 2023
@CodyCBakerPhD CodyCBakerPhD deleted the ci_tests_for_flask_builds_2 branch August 18, 2023 16:46
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