Refactor search tests #6197
Refactor search tests #6197
Conversation
- move test_elastic.py to test_search/test_elastic.py - factor out test_search/test_views.py from test_views.py
|
|
||
| def test_new_user(self): | ||
| # Verify that user has been added to Elastic Search | ||
| docs = query_user(self.user.fullname)['results'] | ||
| assert_equal(len(docs), 1) | ||
| assert len(docs) == 1 |
There was a problem hiding this comment.
I'm afraid this change is not approved. Aside from the stylistic difference, the nose_asserts give more information than the bare asserts unless we are using pytest (which we do not). Please revert these back.
There was a problem hiding this comment.
Okay, rebased out in f0eec53.
|
Pass done. I'll do more once I have a significantly smaller diff to compare. 🐧 I didn't see anything else that went against our existing standards in a serious way, although we obviously won't want to have this PR hanging out too long or it will cause so many merge conflicts. |
Let's do more of those! Ideally we'd remove the redundant "Factory" from the classnames as well, but let's not get *too* uppity here. ;-)
We only need the requires_search decorator on the base class, not on every subclass as well. Plus a couple bugfixes (tests weren't skipped when they should've been).
I made a PR for the first commit, since that was the bulk of the diff in terms of LOC though it's really a simple change. I've updated the description on this PR with a link to the diff between that and this. The individual commits are also suitable for viewing separately. ;-) |
|
I wasn't super concerned about moving the search views to a new file, it was mostly all the asserts that were interspersed between things. This looks like it should be fine once tests pass. It's not going to get merged for at least a day or two, though, even if all is well. |
|
Oh, and don't forget to use gitflow naming for branches: items for develop should be in a |
|
The |
Okay, I've closed #6199 and the associated Jira ticket.
Ah, gotcha. So instead of a separate "QA notes" section (cf.), I will use "Changes" for that ... and I take it from your(?) edit of #5977 (comment) that "Side effects" is for the same audience, QA. 👍 |
I just slowed down and actually read all of "A successful Git branching model" for the first time. Sounds like the original model was looser on naming conventions ("anything except [...]"), with the git-flow scripts enforcing a slightly modified convention, the one we seem to be using. I will endeavor to observe this convention with all diligence in the future! |
|
Cool, thanks. We're not 100% gitflow, and it's more important with hotfixes, but it's a nice double-check even for feature branches. |
#6184 →
Purpose
Clean up our search test suite ahead of extending it with tests for private search (#6184).
Changes
There's nothing for QA to test
Side effects
n/a
Ticket
https://openscience.atlassian.net/browse/OSF-6928