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

Reactivate tests #133

Merged
merged 7 commits into from
May 16, 2022
Merged

Reactivate tests #133

merged 7 commits into from
May 16, 2022

Conversation

jburel
Copy link
Member

@jburel jburel commented May 16, 2022

  • Refactor tests and re-activate testDownloadRelease disabled during the migration to GitHub action.
  • Fixes url to re-activate tests using Jenkins

check that the build is green.

cc @sbesson

@sbesson
Copy link
Member

sbesson commented May 16, 2022

If I understand correctly, there are multiple rationales for the breakage of the download tests: 1- the jobs have been migrated away from ci.openmicroscopy.org and renamed, 2- some of the historical artifacts like OMERO.py are no longer generated as bundles.

Rather than creating a new class and copying the methods, would updating the TestDownload.setup_class directly be an option? The main reason for suggesting this is that it could allow other tests to be re-included and re-increase the library coverage.

@jburel
Copy link
Member Author

jburel commented May 16, 2022

I picked the new class since it was the approach taken in the approved work done in #127 so I assumed that was already discussed.
I can review the setup_class.
I will look at the other tests after

@sbesson
Copy link
Member

sbesson commented May 16, 2022

From #127, I see the feature split the testing into three top-level classes which I understand was aimed at testing different artifact hosting services:

  • the artifactos stored on the Jenkins CI (TestDownload)
  • the binaries hosted at UoD (TestDownloadRelease)
  • the binaries hosted at GitHub (TestDownloadGitHub)

If this is still the preferred solution class from the ongoing investigation in #132, no objection to introducing new classes.

A side note: while some of the test logic is specific to the service hosting the artifact, several methods (testDownloadNonExistingArtifact, testDownloadSym, testDownloadUnzipDir...) effectively some business logic that is independent of where the artifact is retrieved from. In this case, would we like to have these tests executed against each hosting service e.g. inherited from a base test class or only run them for one class?

@jburel
Copy link
Member Author

jburel commented May 16, 2022

organising the tests depending on the services, I think, will make things more readable.
For the second point, since we are dropping certain artifacts it might not be possible to test against all the services but conceptually, testing again each service will be preferable.

The first step is to re-activate the tests. We can re-organise in a follow-up body of work

@jburel jburel changed the title refactor release tests Reactivate tests May 16, 2022
@jburel
Copy link
Member Author

jburel commented May 16, 2022

The tests are now all active. Running unzip tests and such like against each service will probably take too long to run.

@sbesson sbesson merged commit c92cdf9 into ome:master May 16, 2022
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.

None yet

2 participants