TBR: remove the extra values which was overshadowing the test result #22483
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| /// this rate is set to 0.28), since during the end to end tests there are | ||
| /// more components on the screen which are not related to the functionality | ||
| /// under test ex: a blinking cursor. | ||
| const double kMaxDiffRateFailure = 0.5 / 100; // 0.5% |
There was a problem hiding this comment.
can we limit this to canvaskit only ?
There was a problem hiding this comment.
thanks. I'll also discuss this as an option, tomorrow with @yjbanov. As we discussed offline the options we have:
- increasing the diff
- applying different diff for canvaskit
- merging different screenshot files for canvaskit
- (long term, after migration) uploading with different parameters to gold.
Since tests are returning unreliable results now, I'd like to keep this till tomorrow.
There was a problem hiding this comment.
My guess is, because the tests have been silently failing, as we were upgrading CanvasKit to new versions and weren't updating the screenshots, they slowly deviated from the goldens. Updating the screenshots might do the trick.
There was a problem hiding this comment.
Could un-updated goldens be causing this LWE build failure?
https://ci.chromium.org/p/flutter/builders/try/Linux%20Web%20Engine/11833?
nturgut
changed the title
|
@yjbanov merging this one as TBR. Since all integration tests are returning exit code 0 without this change (unit tests are not effected) I'd like this to work ASAP. |
|
https://ci.chromium.org/p/flutter/builders/prod/Linux%20Web%20Engine/2640 |
|
The revert encountered the same problems so this PR was not the cause. Perhaps something changed in the framework that it was being rolled into. |
| return numberOfFailedTests == 0; | ||
| print('INFO: ${numberOfTestsRun} tests run. ${_numberOfPassedTests} passed ' | ||
| 'and ${_numberOfFailedTests} failed.'); | ||
| return _numberOfFailedTests == 0; |
There was a problem hiding this comment.
This LGTM as an emergency fix, but we also need a test that fails intentionally to verify that when we need to fail we do. We can borrow ideas from other tools that have failure tests:
There was a problem hiding this comment.
Great, thanks! I was also thinking about it yesterday. I think that's a very important test case to have.
* 03ff8a4 TBR: remove the extra values which was overshadowing the test result (flutter/engine#22483) * e9b6a42 Roll Skia from a06b63c56ecd to 8ead30d51c86 (1 revision) (flutter/engine#22487) * ee70051 Roll Fuchsia Linux SDK from fULjPqtx9... to B4PaMsNWM... (flutter/engine#22490) * 4cbb684 Roll Dart SDK from 3e502e0c7e04 to 41893ff76b0f (1 revision) (flutter/engine#22491) * 38b6c22 PlatformViewIOS CreateExternalViewEmbedder refactor (flutter/engine#22353) * bbcf19a Roll Skia from 8ead30d51c86 to 011218edb590 (4 revisions) (flutter/engine#22493)
…lutter#22483) * remove the extra values which was overshadowing the test result * increase difference. canvaskit gives different results * increase difference more. firefox gave 1.7 diff. add comments
4 participants
Description
Felt is reporting exitcode 0 even though one or more tests fails. Remove the wrong integer values.
Related Issues
fixes: flutter/flutter#70400
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.Reviewer Checklist
Breaking Change
Did any tests fail when you ran them? Please read handling breaking changes.