Skip to content

Shrink our gigantic tests! #115

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

Merged
merged 5 commits into from
May 17, 2016
Merged

Shrink our gigantic tests! #115

merged 5 commits into from
May 17, 2016

Conversation

wafflespeanut
Copy link
Contributor

@wafflespeanut wafflespeanut commented May 6, 2016

This makes use of a simple wrapper around the JSON object and marks stuff whenever we 'get' something from the dictionary (using []). This can help shrink the gigantic tests we currently have (which has a hell lot of unused key/val)

@highfive
Copy link
Collaborator

highfive commented May 6, 2016

Heads up! This PR modifies the following files:

  • @jdm: helpers.py, eventhandler.py, test.py, json_cleanup.py

@wafflespeanut
Copy link
Contributor Author

gotta fix the flake8 stuff...

@wafflespeanut
Copy link
Contributor Author

It might be worth taking a look at the failed Travis build! 😄

@wafflespeanut
Copy link
Contributor Author

@jdm I think it's still buggy, but well, this is just a prototype. I'm curious whether this would be useful if I make it to work.

@highfive
Copy link
Collaborator

highfive commented May 7, 2016

New code was committed to pull request.

@wafflespeanut
Copy link
Contributor Author

Now, this is progress! (still, 3 tests fail)

@highfive
Copy link
Collaborator

highfive commented May 8, 2016

New code was committed to pull request.

@wafflespeanut
Copy link
Contributor Author

All the tests pass now, but I had to put some type conversions to integrate the lint.

@wafflespeanut
Copy link
Contributor Author

After trying to integrate this into highfive (for a while now), I realize that the entire problem lies on creating the tests, where we read the JSON files twice (one for the initial/expected stuff), and the other for payload. I think I should modify the linter to take objects instead of going directly for the file.

@highfive
Copy link
Collaborator

highfive commented May 8, 2016

New code was committed to pull request.

@wafflespeanut
Copy link
Contributor Author

  • fixed the linter for flagging only the stuff in payload
  • avoided reading the test JSONs twice
  • added flag to warn/overwrite the nodes in test files

only the command-line argument is left to go...

@highfive
Copy link
Collaborator

highfive commented May 8, 2016

New code was committed to pull request.

@wafflespeanut
Copy link
Contributor Author

I'm finally satisfied with the output 😄

@wafflespeanut wafflespeanut changed the title [WIP] Shrink our gigantic tests! Shrink our gigantic tests! May 8, 2016
@bors-servo
Copy link

☔ The latest upstream changes (presumably #118) made this pull request unmergeable. Please resolve the merge conflicts.

@highfive
Copy link
Collaborator

New code was committed to pull request.

@highfive
Copy link
Collaborator

New code was committed to pull request.

@highfive
Copy link
Collaborator

New code was committed to pull request.

2 similar comments
@highfive
Copy link
Collaborator

New code was committed to pull request.

@highfive
Copy link
Collaborator

New code was committed to pull request.

@wafflespeanut
Copy link
Contributor Author

This is ready for review!

@highfive
Copy link
Collaborator

New code was committed to pull request.

@highfive
Copy link
Collaborator

New code was committed to pull request.

@@ -55,7 +55,7 @@ def on_pr_opened(self, api, payload):
pr = payload["pull_request"]
# If the pull request already has an assignee,
# don't try to set one ourselves.
if pr["assignee"] != None:
if pr["assignee"] != None: # NOQA (silence flake8 here)
Copy link
Member

Choose a reason for hiding this comment

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

What is it complaining about?

Copy link
Contributor Author

@wafflespeanut wafflespeanut May 16, 2016

Choose a reason for hiding this comment

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

flake8 prefers is not None, but we need != to call our overridden __ne__ method (the former checks for an exact match of the object, which will fail during testing)

@jdm
Copy link
Member

jdm commented May 16, 2016

The need for str(foo) and int(foo) in various places is disappointing. Is there no way around that?

@highfive
Copy link
Collaborator

New code was committed to pull request.

@wafflespeanut
Copy link
Contributor Author

wafflespeanut commented May 17, 2016

True, they are bad, but I couldn't find a way around them. I've fixed some of them, and now we have only three situations (in regex matching) where str(foo) occurs. Python's re methods look pretty strict, that they accept only string/unicode, which prevents us from overriding anything 😞

@jdm jdm merged commit 7f3112c into servo:master May 17, 2016
@jdm
Copy link
Member

jdm commented May 17, 2016

Thanks!

@wafflespeanut wafflespeanut deleted the chop_tests branch May 18, 2016 02:33
@wafflespeanut
Copy link
Contributor Author

Thanks for reviewing it :)

Mark-Simulacrum pushed a commit to Mark-Simulacrum/highfive that referenced this pull request Sep 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants