8000 Backport static analysis fixes to 1.6 by veewee · Pull Request #337 · azjezz/psl · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Backport static analysis fixes to 1.6 #337

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
Merged
merged 4 commits into from
Feb 23, 2022
Merged

Backport static analysis fixes to 1.6 #337

merged 4 commits into from
Feb 23, 2022

Conversation

veewee
Copy link
Collaborator
@veewee veewee commented Feb 23, 2022 •
edited
Loading

Backported these fixes to resolve Type\shape() issues:

@veewee veewee requested a review from azjezz February 23, 2022 07:38
@coveralls
Copy link
coveralls commented Feb 23, 2022 •
edited
Loading

Pull Request Test Coverage Report for Build 1887119618

  • 4 of 4 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 1553437260: 0.0%
Covered Lines: 2775
Relevant Lines: 2775
💛 - Coveralls

@veewee veewee force-pushed the 1.6-backported-sa-fixes branch 2 times, most recently from e51707a to 87b366f Compare February 23, 2022 08:02
@veewee
Copy link
Collaborator Author
veewee commented Feb 23, 2022 •
edited
Loading

@azjezz any clue on the failing unit test?
It only fails on mac for some reason, and it didn't change in versions above...
Looks like a rounding issue

1) Psl\Tests\Math\TanTest::testTan with data set #0 (-3.380515006246586, 5.0)
Failed asserting that -3.380515006246585 is identical to -3.380515006246586.

/Users/runner/work/psl/psl/tests/Psl/Math/TanTest.php:17

Should be "-3.380515006246586" on all versions though : https://3v4l.org/Bq0B4
My mac also states it is "-3.380515006246585". So might be a mac issue, but then again - it should fail on 2.x and 1.9.x as well

@azjezz
Copy link
Owner
azjezz commented Feb 23, 2022

I'm guessing this fails for PHP 7.4, but not +8.0? 🤔 no idea honestly.

If it's just this test case, we can suppress it for 1.6 branch.

@veewee
Copy link
Collaborator Author
veewee commented Feb 23, 2022 •
edited
Loading

Tested it on all php versions on Mac and got the same error.
Don't understand why it does work on never versions of PSL, since nothing changed to the code or test..

Will suppress it on this branch

@veewee veewee force-pushed the 1.6-backported-sa-fixes branch from 0df8d3f to f6d7bef Compare February 23, 2022 11:57
@veewee veewee force-pushed the 1.6-backported-sa-fixes branch from f6d7bef to 8a9f005 Compare February 23, 2022 12:04
@azjezz azjezz added this to the 1.6.0 milestone Feb 23, 2022
@azjezz azjezz added Priority: Medium This issue may be useful, and needs some attention. Type: Maintenance Updating phrasing or wording to make things clearer or removing ambiguity Status: Completed Nothing further to be done with this issue. Awaiting to be closed by the requestor out of politeness labels Feb 23, 2022
@azjezz azjezz merged commit 079e47c into azjezz:1.6.x Feb 23, 2022
@azjezz
Copy link
Owner
azjezz commented Feb 23, 2022

@veewee can you release a new 1.6?

@veewee
Copy link
Collaborator Author
veewee commented Feb 23, 2022

Done! Thanks.

@azjezz
Copy link
Owner
azjezz commented Feb 24, 2022

@veewee the MacOS issue exists in 2.0.x branch as well, showed up now after i ran CI :/

@veewee
Copy link
Collaborator Author
veewee commented Feb 24, 2022

Probably related with newer Mac versions and underlying libraries then... We could round the result to 5 digits in the test cases or report it to internals? The rounding issue doesn't seem that dramatic to me though. But then again: I haven't used cos,sin,tan in ages

@jrmajor
Copy link
Contributor
jrmajor commented Feb 24, 2022 •
edited
Loading

@veewee There's a dedicated PHPUnit method for this use case: assertEqualsWithDelta(). Unfortunately, there's nothing like assertSameWithDelta().

Edit: looking at the source code, I think assertEquals() has a default delta: https://github.com/sebastianbergmann/comparator/blob/926815b015c4c459f8ec5185328eb34ade07c51f/src/DoubleComparator.php#L53-L55.

@azjezz
Copy link
Owner
azjezz commented Feb 24, 2022

assertEqualsWithDelta()

we shouldn't use this, the float value is correct, something weird is going on in MacOs that results in incorrect result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@azjezz azjezz azjezz approved these changes

Assignees

@veewee veewee

Labels
Priority: Medium This issue may be useful, and needs some attention. Status: Completed Nothing further to be done with this issue. Awaiting to be closed by the requestor out of politeness Type: Maintenance Updating phrasing or wording to make things clearer or removing ambiguity
Projects
None yet
Milestone
1.6.0
Development

Successfully merging this pull request may close these issues.
4 participants
@veewee @coveralls @azjezz @jrmajor
0