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.

Sign up for GitHub

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

Jump to bottom

[23.1] Fix tool remote test data #16311

Merged
merged 7 commits into from
Jun 28, 2023
Merged

[23.1] Fix tool remote test data #16311

merged 7 commits into from
Jun 28, 2023

Conversation

davelopez
Copy link
Contributor

@davelopez davelopez commented Jun 26, 2023 •
edited
Loading

Follow up on #15510 and #16291

This PR reverts most of the changes in #15510 as that approach was moving too much unnecessary data back and forth to Galaxy.

The new implementation is simpler and makes use of the pasted URL upload for inputs. The only difference in functionality with the previous implementation is that there is no checksum verification for remote inputs for now as we are still using the upload1 tool to upload test datasets (we can use the __DATA_FETCH__ that supports checksums to upload test data in future improvements).

How to test the changes?

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. Make sure you have a tool with inputs and outputs defined in location attributes (like test/functional/tools/remote_test_data_location.xml and there is no local test data already present.
    2. Make sure galaxy is running with this branch
    3. Call galaxy-tool-test with the appropriate parameters to test your tool

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@github-actions github-actions bot added this to the 23.2 milestone Jun 26, 2023
@davelopez davelopez added kind/refactoring cleanup or refactoring of existing code, no functional changes kind/feature labels Jun 26, 2023
@davelopez davelopez force-pushed the fix_tool_remote_test_data branch 2 times, most recently from 80422df to d484dab Compare June 27, 2023 07:35
@davelopez davelopez mentioned this pull request Jun 27, 2023
Merged
5 tasks
@bernt-matthias
Copy link
Contributor

Anything I can do to help? Maybe test?

@davelopez
Copy link
Contributor Author

Sure! Testing would be ideal, or any other improvements too :)

bernt-matthias
bernt-matthias reviewed Jun 27, 2023
View reviewed changes
Copy link
Contributor

@bernt-matthias bernt-matthias left a comment

Choose a reason for hiding this comment

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

I successfully tested the code with planemo (using the calisp tool from the proteomics tool repo where I already prepared for test data download).

test/functional/tools/sample_tool_conf.xml
@@ -87,7 +87,6 @@
<tool file="is_valid_xml.xml" />
<tool file="expression_tools/parse_values_from_file.xml"/>
<tool file="expression_tools/pick_value.xml" />
<tool file="remote_test_data_location.xml" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not keep it?

Copy link
Contributor Author

@davelopez davelopez Jun 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I guess it doesn't need to run as part of the framework functional tool tests anymore. Looks like It is being tested here though... https://app.circleci.com/pipelines/github/galaxyproject/galaxy/32282/workflows/a028ae07-de5c-40b6-9ff1-bb865729bdd4/jobs/193407?invite=true#step-105-413
But I'm happy to add it back if it makes sense, I'm just not sure where is the correct place for testing this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is only linting the tools (which is also important). Otherwise I would be wondering about the test runtime of 192.47 seconds :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh true! lol

lib/galaxy/tool_util/xsd/galaxy.xsd Outdated Show resolved Hide resolved
@bernt-matthias
Copy link
Contributor

@mvdbeek any chance that we get this in 23.1?

@dannon
Copy link
Member

dannon commented Jun 27, 2023

@bernt-matthias It's fine to get this in as a bugfix during the freeze.

@davelopez
Copy link
Contributor Author

Should I retarget 23.1 as a "fix" then? or it would be enough just to publish a new version of the galaxy-tool-util package after merge and use it in Planemo?

@mvdbeek
Copy link
Member

mvdbeek commented Jun 28, 2023

Let's do 23.1 then. It isn't really necessary since we can do dev releases, but I guess it's also nice not to require dev releases in non-dev planemo versions.

@davelopez davelopez marked this pull request as draft June 28, 2023 10:25
@davelopez davelopez force-pushed the fix_tool_remote_test_data branch from de68bfa to d8937d4 Compare June 28, 2023 10:33
@davelopez davelopez changed the base branch from dev to release_23.1 June 28, 2023 10:33
@davelopez davelopez changed the title Fix tool remote test data [23.1] Fix tool remote test data Jun 28, 2023
@davelopez davelopez modified the milestones: 23.2, 23.1 Jun 28, 2023
@davelopez davelopez marked this pull request as ready for review June 28, 2023 11:31
@mvdbeek mvdbeek merged commit 71d4823 into galaxyproject:release_23.1 Jun 28, 2023
@davelopez
Copy link
Contributor Author

Thank you @mvdbeek!

@davelopez davelopez deleted the fix_tool_remote_test_data branch June 28, 2023 13:31
@davelopez davelopez mentioned this pull request Aug 11, 2023
Merged
2 tasks
@bernt-matthias bernt-matthias mentioned this pull request Dec 18, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bernt-matthias bernt-matthias bernt-matthias left review comments

Assignees
No one assigned
Labels
area/testing area/tool-framework kind/feature kind/refactoring cleanup or refactoring of existing code, no functional changes
Projects
None yet
Milestone
23.1
Development

Successfully merging this pull request may close these issues.
4 participants
@davelopez @bernt-matthias @dannon @mvdbeek