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

Webhooks: add tests #3469

Merged
merged 5 commits into from
Jan 29, 2017
Merged

Webhooks: add tests #3469

merged 5 commits into from
Jan 29, 2017

Conversation

anatskiy
Copy link
Contributor

@anatskiy anatskiy commented Jan 24, 2017 •
edited
Loading

Once Webhooks have been introduced, tests were missing. Now I fixed it. Also, WebhooksRegistry doesn't consider junk files (like .DS_Store) and the demo folder as a webhook directory anymore.

@anatskiy
- add Webhooks tests
809c087 
- small code improvements
@galaxybot galaxybot added this to the 17.05 milestone Jan 24, 2017
@bgruening
Copy link
Member

Don't worry about the failing tests, that will be fixed in an other PR.
Thanks @anatskiy

@martenson
Copy link
Member

@galaxybot test this

@jmchilton
Copy link
Member

Thanks for the contribution - I'm going to open a PR to fix this up a bit. It is close but you...

  • Shouldn't need to modify Galaxy to treat your test data specially (this is a smell indicating a test problem).
  • API tests shouldn't modify app IMHO - they should treat Galaxy as somewhat immutable so they can run on remote Galaxy instances. Really no test should modify app in this way because it violates Galaxy's abstractions. If you need Galaxy with a specific configuration - you should write an integration test that configures Galaxy this way from the get go - this way you are not violating Galaxy's abstractions and you also have the benefit of testing Galaxy's configuration parsing code.

These should be really easy to fix but there isn't a lot of test documentation so I'm going to open a PR against your repository - I hope that is okay.

@jmchilton
Treat demo/test webhooks like demo/test tools.
867e8dc 
- Populate test server with demo webhooks by default.
- Eliminate special logic needed to treat the demo directory differently.
- Document how to enable the demo webhooks in galaxy.ini.sample.
- Populate the demo webhooks when starting Galaxy with GALAXY_RUN_WITH_TEST_TOOLS=1 set.
@jmchilton
Update webhooks API test to set expectations independent of implement…
8b30d87 
…ation.

It may seem like this is testing less since fewer API calls are being made - but when you use the backend to generate the data you trust in the test case - the correlation lands up meaning relatively less is tested. For instance, if there is some problem arises in the to_dict method of a webhook - it is going to arise in both the implementation and in the test case and so the test case will just be asserting that the response is broken ... and it will be. Testing more concrete things is also good for ensuring backward compatibility.
@anatskiy
Copy link
Contributor Author

@jmchilton yeah, sure, no problem.

I spent I lot of time trying to figure out how to change the webhooks_directory path but couldn't find anything. That's why I had to re-use the WebhooksRegistry in such a manner.

@jmchilton
Copy link
Member

I pushed a merge against dev to resolve conflicts in the PR. This is really nice - thanks for the tests!

@jmchilton jmchilton merged commit 4cb7617 into galaxyproject:dev Jan 29, 2017
@jmchilton
Copy link
Member

@bgruening In response to some out of band comments on this pull request I get that these are potentially useful plugins on there own - and I'm willing to move them outside of $GALAXY_ROOT/test for sure. I just don't think config/plugins/webhooks/demo is appropriate because I feel like each directory in config/plugins/webhooks/ should be a plugin. Also I guess tests shouldn't depend on things in config/plugins/ if it can be avoided since things may be removed from there by deployers and the tests should still pass.

If you want to put the useful plugins in contrib/plugins/webhooks or something - I would be fine with that - but I think they are fine in test for now as well. Let me know if you'd like to keep discussing this.

@bgruening
Copy link
Member

I think it's ok. You have added a nice comment into the galaxy.ini and people can also add the path to the test dir to galaxy.ini. It ok as it is. We loose a little bit of visibility but I guess as soon as people are confident with this plugin infrastructure we should advertise it more and enable PhD comics on main 😛

@anatskiy anatskiy deleted the feature/webhooks_tests branch October 27, 2017 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
area/testing area/UI-UX
Projects
None yet
Milestone
17.05
Development

Successfully merging this pull request may close these issues.
5 participants
@anatskiy @bgruening @martenson @jmchilton @galaxybot