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.

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

tests: fix B015 #682

Merged
merged 2 commits into from Jan 28, 2021
Merged

tests: fix B015 #682

merged 2 commits into from Jan 28, 2021

Conversation

henryiii
Copy link
Member

@henryiii henryiii commented Jan 28, 2021

B015 Pointless comparison. This comparison does nothing but wastes CPU instructions. Remove it.

This is a followup to #681; this time fixing a warning from one of my favorite flake8 extensions, bugbear. The warning (above) tells you to remove pointless comparisons, but 95% of the time, such as all the cases here, they tend to be missed asserts in PyTest.

B015 Pointless comparison. This comparison does nothing but wastes CPU instructions. Remove it.
@jpivarski
Copy link
Member

Although technically the way these were caught was by "pointless comparisons," they're not pointless—they're attempts at assertions that failed to get the word "asset" (or somehow lost it). Similarly, before I added the rule that "the truth value of an array is ambiguous," there had been assertions that always returned true, even if the arrays on both sides of the == didn't match. (They merely had to return a non-empty sequence, even if that sequence was full of "false" values.) That's probably why NumPy introduced that rule.

This automated check has revealed two failing tests. Both contain a trivial thing to fix: the string representation of a floating point number has a trailing .0, but one of them also has a substantial error: ak.from_json(file) was recently modified to accept a sequence of JSON documents into an array. In this particular file, it has only one document and should be one level less nested than it is—the new rule about accepting a sequence of JSONs shouldn't make a single JSON into an item in an array.

This test from way back in the "0018" era would have caught this introduced error, but it was missing the word assert. It's a great automated check!

I'll fix this tomorrow when I get to my computer, unless @ianna gets to it first. (This is in the new multi-line JSON handling. If only one full JSON document is observed, it should not be considered the first in a series, but the entirety of that single document.)

@henryiii
Copy link
Member Author

I'm using my fork, so gh pr checkout 682 will check this out if you use the GitHub command line tool without any fuss. If put something in that I think others will be likely to edit, I try to use the main repo, generally, just to simplify collaboration. I didn't know this would actually catch a bug. :)

@henryiii
Copy link
Member Author

I've actually had a discussion with the author of this check on special cases, by the way. There is a very rare, special case where you have to use a # noqa for it (no questions asked, didn't realize that at first): if you have a comparison that throws an error that you want to catch in your tests. But yes, it's fantastic and one of my favorites. :)

@jpivarski
Copy link
Member

I had been thinking that this was a bug introduced by PR #543, but I was wrong. It wasn't reading a single JSON document as though it were a JSONs stream, it was writing a single JSON document with one level too deep nesting.

I did some forensics on it: there was a time when the tojson_part was changed from assuming it was inside a beginlist()/endlist() pair to creating the beginlist()/endlist() pair if it's a list type. That means an update in two places: inside tojson_part and outside of it in the Content::tojson that calls it. (Here is what it looked like in olden times, just after these functions were introduced, and here is what it looked like right before I put in this fix. The builder.beginlist()/builder.endlist() was removed from the overload that creates a string to agree with the new definition within tojson_part, but it was not removed from the overload that writes to a file.)

When this change was put in, I must have been testing the string output because that's easy to write a test around. I would have been assuming that the file version was okay because there were tests of JSON input and output "since forever." So I was explicitly relying on the test that apparently didn't have the word assert before it.

@henryiii
Copy link
Member Author

It's green. :)

@jpivarski jpivarski merged commit 903480f into scikit-hep:main Jan 28, 2021
@henryiii henryiii mentioned this pull request Jan 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants