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

Cleanup actions #160

Merged
merged 2 commits into from
Apr 30, 2024
Merged

Cleanup actions #160

merged 2 commits into from
Apr 30, 2024

Conversation

austinorr
Copy link
Contributor

No description provided.

@PaulDudaRESPEC
Copy link
Member

Yeah, since these are causing trouble let's take them out for now, with the intent of restoring them when ready.

@PaulDudaRESPEC PaulDudaRESPEC merged commit 2e414f9 into respec:develop Apr 30, 2024
1 check failed
@austinorr austinorr deleted the cleanup-actions branch April 30, 2024 23:36
@rburghol
Copy link
Contributor

rburghol commented May 6, 2024

I wanted to add some context to these changes, and first off, thanks @austinorr for spying that 6 hour runtime! That would kill our free tier of testing in a jiffy. Rather than deleting these tests, @PaulDudaRESPEC, I would nominate that we disable them rather than delete (see "Disabling a Test" in our main testing issue: #135) unless they are superfluous:

  • This conda install test is IMO an important piece to verify the install process listed on the front page of the project, namely the conda route:
    • This hang-up failure may have something to tell us about the action code that was introduced in a commit 2 weeks ago
  • The dev install test:
    • if we still need to have a dev install separate from the current version, I think that it is prudent to keep it in a separate testing Action. The reason being, if we go to the place where we want to make these tests necessary for allowing a merge, then we will want to be able to separate the dev from the current, since we may very well accept commit that breaks the dev install but still passes the live install Action.

image

@austinorr
Copy link
Contributor Author

@rburghol tests and actions have been revised and improved in #156.

the conda test you mentioned above (1) tests ‘if things also run in an active conda environment’ — that test probably already covered in the conda project.

if we do our job, then our code will run fine in any environment manager that is also doing its job.

for the second item, tests written into gh actions for our master branch will run when someone opens a PR against master, and similarly for our develop branch code. Actions are committed into the git repo (not configured as part of the overall website) and so, if I understand your concerns correctly, they will already do the thing you describe by default and we don’t need a second dev action.

@rburghol
Copy link
Contributor

rburghol commented May 6, 2024

Hey @austinorr thanks for the responses:

  • (1) The conda test was testing runnability, but also it tested installability, which is where the failure was occurring. A couple other points in favor of having the separate tests for separate install paths:
    • The assumption that if we do our job, it will run fine may be correct, though the tests are there to verify that we did our job right,
    • The installability is still a big question mark IMO (though this may simply be a lack of proper test coding). Regardless, because local machine dev and testing can be different than a fresh install, if we still have the conda install instructions as our recommended way to do this, we oughtta have test coverage for it.
  • (2) I think I did not explain this well enough:
    • I think I understand how the tests apply to different branches -- the Action scripts themselves actually outline which branches trigger the test, so we can have different tests for different branches if we want.
    • But to the current point, in our previous setup/install path, there were 2 different yml files to install variants, 1 which was limited to the 3.9.* python version, and the 2nd which was exploring some later versions (due to problems with numba IIRC). So, the 2nd files was not for a separate branch, but rather, for a separate underlying system of requirements and python versions.
    • The need for separate python/numba version testing may be gone, but if it's not gone, we should have test coverage for it. Though honestly, even if the current numba version snafu is no longer, it is probably not a bad idea to always have a forward looking install test which looks at cutting edge versions of python to see if any new problems are on the horizon.

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.

4 participants