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

[WIP] Update tests for manuscripts2 #71

Merged
merged 3 commits into from
Jul 19, 2018
Merged

[WIP] Update tests for manuscripts2 #71

merged 3 commits into from
Jul 19, 2018

Conversation

aswanipranjal
Copy link
Contributor

This PR updates the tests by adding the functionality to query elasticsearch and compare the responses with predefined results.

@jgbarah jgbarah requested review from jgbarah and acs July 10, 2018 21:40
jgbarah
jgbarah approved these changes Jul 10, 2018
View reviewed changes
Copy link
Contributor

@jgbarah jgbarah left a comment

Choose a reason for hiding this comment

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

Looks good to me. But I would prefer somebody else having a look too.

@aswanipranjal aswanipranjal mentioned this pull request Jul 13, 2018
Merged
@aswanipranjal aswanipranjal changed the title Update tests for manuscripts2 [WIP] Update tests for manuscripts2 Jul 16, 2018
@aswanipranjal
Copy link
Contributor Author

Kindly don't merge this PR yet. I found a bug and I am almost done fixing it.

@aswanipranjal
Copy link
Contributor Author

@jgbarah the tests now use actual data to check if the functions work properly or not. Kindly review?

@aswanipranjal
Copy link
Contributor Author

@acs @valeriocos can you please also review this? Thanks!

valeriocos
valeriocos approved these changes Jul 18, 2018
View reviewed changes
Copy link
Member

@valeriocos valeriocos left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM. I have just a minor question about the copyright, should be CHAOSS or Bitergia, like in other files (e.g., https://github.com/chaoss/grimoirelab-manuscripts/blob/master/manuscripts/report.py#L6) ?

@jgbarah
Copy link
Contributor

jgbarah commented Jul 18, 2018

I have just a minor question about the copyright, should be CHAOSS or Bitergia

Since @aswanipranjal is not working for Bitergia, but a GSoC intern working in CHAOSS, I think CHAOSS is more appropriate...

@jgbarah
Copy link
Contributor

jgbarah commented Jul 18, 2018

@aswanipranjal could you please rebase, and I will merge it?

@aswanipranjal
Update Query class: set start and end date filters directly into sear…
a2f1281 
…ch object
@aswanipranjal
Update tests for manuscripts2
b41525d 
- Using elasticsearch to create indices and query them
- add expected dummy data in tests/data folder
- update each test to query ES and compare results with dummy data
- update .travis.yml file for integrated tests
@aswanipranjal
Update tests as an effect of chaoss#74
ef9fdf9 
- Now calling get_aggs as a method of Query class and not as an
independent function
@aswanipranjal
Copy link
Contributor Author

Done, @jgbarah.

@aswanipranjal
Copy link
Contributor Author

aswanipranjal commented Jul 18, 2018 •
edited
Loading

@jgbarah can we please merge this? It'll be easier to re-name the files then.

@jgbarah jgbarah merged commit 0968515 into chaoss:master Jul 19, 2018
@aswanipranjal
Copy link
Contributor Author

Thank you @jgbarah!!

@aswanipranjal aswanipranjal deleted the updating-manuscripts2-tests branch July 21, 2018 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@valeriocos valeriocos valeriocos approved these changes

@jgbarah jgbarah jgbarah approved these changes

@acs acs Awaiting requested review from acs

@alpgarcia alpgarcia Awaiting requested review from alpgarcia

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aswanipranjal @jgbarah @valeriocos