sympy / sympy-bot-old

SymPy pull request helper
http://reviews.sympy.org/
Other
24 stars 16 forks source link

Revert "Add the number of xpassed tests to summary." #72

Closed asmeurer closed 12 years ago

asmeurer commented 12 years ago

This reverts commit 2959528e1fd12b9dfd36a1ffb869dad24491aad8.

This information is not important enough to put in the summary (it can go in the report, if you want).

Conflicts:

sympy-bot
testrunner.py

See #69.

smichr commented 12 years ago

Can you give a snippet of a output before and after this commit?

asmeurer commented 12 years ago

Summary before:

Test results html report: (report was not uploaded)

*Interpreter:*   /sw/bin/python  (2.5.0-final-0)
*Architecture:* Darwin (64-bit)
*Cache:*        yes
*Test command:* **./bin/test core**
*master hash*: 9dc06ee4d65f46bbe76768b468ecf69f38cb73f7
*branch hash*: 3d819521a1b16d6c4389376dcd06c6708ccd2b62
*XPassed tests:* 3

**Summary:** All tests have passed.

Automatic review by [sympy-bot](https://github.com/sympy/sympy-bot).

Which looks like

Test results html report: (report was not uploaded)

Interpreter: /sw/bin/python (2.5.0-final-0) Architecture: Darwin (64-bit) Cache: yes Test command: ./bin/test core master hash: 9dc06ee4d65f46bbe76768b468ecf69f38cb73f7 branch hash: 3d819521a1b16d6c4389376dcd06c6708ccd2b62 XPassed tests: 3

Summary: All tests have passed.

Automatic review by sympy-bot.


Summary after

Test results html report: (report was not uploaded)

*Interpreter:*   /sw/bin/python  (2.5.0-final-0)
*Architecture:* Darwin (64-bit)
*Cache:*        yes
*Test command:* **./bin/test core**
*master hash*: 9dc06ee4d65f46bbe76768b468ecf69f38cb73f7
*branch hash*: 3d819521a1b16d6c4389376dcd06c6708ccd2b62

**Summary:** All tests have passed.

Automatic review by [sympy-bot](https://github.com/sympy/sympy-bot).

which looks like

Test results html report: (report was not uploaded)

Interpreter: /sw/bin/python (2.5.0-final-0) Architecture: Darwin (64-bit) Cache: yes Test command: ./bin/test core master hash: 9dc06ee4d65f46bbe76768b468ecf69f38cb73f7 branch hash: 3d819521a1b16d6c4389376dcd06c6708ccd2b62

Summary: All tests have passed.

Automatic review by sympy-bot.


I just removed the "XPassed" line.

smichr commented 12 years ago

We should be paying attention to un-xpassing tests, however, so shouldn't we see that info up front?

asmeurer commented 12 years ago

But this is a very rare occurrence. Perhaps we could put it there only if the number is larger than it usually is. This would require keeping track of this information in the app engine.

certik commented 12 years ago

I also don't find the information about the xpassed tests important enough to be put into the comments. I do like the other information though, like the platform and the hashes, so that one can see exactly what was tested against what on what platform.

certik commented 12 years ago

As such, instead of reverting the whole commit, I would keep the code there, just don't show it (so that we can refactor it later into showing in the app engine report).

smichr commented 12 years ago

OK, change me to +0

asmeurer commented 12 years ago

Personally, I think all the code I modified here needs to be refactored a little. The functions formulate_review, and others are just growing in the number of arguments they take. They should just take a single dictionary (**kwargs) of everything, and then choose from that what they want to use.

I do agree that this information is still useful, but should be put only on the full report. I didn't have time to do more than revert the commit, though. If you want to fix it better, go ahead. Otherwise, I will get to it when I can.

certik commented 12 years ago

It should be a one line fix, so I'll try to do it soon.

asmeurer commented 12 years ago

I've redone this.