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

Linux Web Framework tests failing #91432

Closed
zanderso opened this issue Oct 7, 2021 · 16 comments
Closed

Linux Web Framework tests failing #91432

zanderso opened this issue Oct 7, 2021 · 16 comments
Assignees
Labels
a: tests "flutter test", flutter_test, or one of our tests P1 High-priority issues at the top of the work list platform-web Web applications specifically

Comments

@zanderso
Copy link
Member

zanderso commented Oct 7, 2021

https://ci.chromium.org/ui/p/flutter/builders/prod/Linux%20SDK%20Drone/92478/overview

Among possibly other failures it looks like there might be an issue with the code comparing goldens (?)

@mdebbar @yjbanov

05:27 +620 ~31 -2: test/cupertino/switch_test.dart: Switch renders correctly before, during, and after being tapped                                                                                    [+3479 ms] Discovered flutter_test_config.dart in /b/s/w/ir/x/w/flutter/packages/flutter/test
[   +1 ms] Compiling file:///b/s/w/ir/x/t/flutter_tools.JCITJV/flutter_web_platform.QNOXQB/listenerPZNWUU/listener.dart
[  +10 ms] <- recompile file:///b/s/w/ir/x/t/flutter_tools.JCITJV/flutter_web_platform.QNOXQB/listenerPZNWUU/listener.dart b7a6d512-bca1-42b8-a257-2704ff9d92c9
[        ] file:///b/s/w/ir/x/t/flutter_tools.JCITJV/flutter_web_platform.QNOXQB/listenerPZNWUU/listener.dart
[        ] file:///b/s/w/ir/x/w/flutter/packages/flutter/.dart_tool/flutter_build/generated_main.dart
[        ] <- b7a6d512-bca1-42b8-a257-2704ff9d92c9
[ +139 ms] <- accept
[        ] <- reset
[        ] Compiling file:///b/s/w/ir/x/t/flutter_tools.JCITJV/flutter_web_platform.QNOXQB/listenerPZNWUU/listener.dart took 151ms
[   +7 ms] Preparing to send command: {"imageFile":"/b/s/w/ir/x/t/flutter_tools.JCITJV/flutter_web_platform.QNOXQB/imageDYNBLU/image","key":"switch.tap.off.png","update":false}
[ +314 ms] <<< [ERROR:flutter/shell/testing/tester_main.cc(400)] Unhandled exception
[        ] <<< Exception: Null check operator used on a null value
[        ] <<< Stack trace: #0      markTestSkipped (package:test_api/src/scaffolding/utils.dart:51:56)
[        ] <<< #1      FlutterSkippingFileComparator.compare (package:flutter_goldens/flutter_goldens.dart:388:5)
[        ] <<< #2      main.<anonymous closure> (file:///b/s/w/ir/x/t/flutter_tools.JCITJV/flutter_web_platform.QNOXQB/listenerPZNWUU/listener.dart:29:53)
[        ] <<< <asynchronous suspension>
[        ] <<< #3      testExecutable (package:flutter_goldens/flutter_goldens.dart:42:3)
[        ] <<< <asynchronous suspension>
@zanderso zanderso added platform-web Web applications specifically P0 Critical issues such as a build break or regression labels Oct 7, 2021
@mdebbar
Copy link
Contributor

mdebbar commented Oct 7, 2021

I don't see anything suspicious in the recent engine or framework PRs. I'll keep looking and try to repro the error locally.

@mdebbar mdebbar self-assigned this Oct 7, 2021
@mdebbar
Copy link
Contributor

mdebbar commented Oct 7, 2021

Actually, I spoke too soon. This is the PR from the framework that introduced the issue: #91332 (cc @Hixie). The PR doesn't do anything wrong but it introduces call to markTestSkipped which fails on web because Invoker.current is null for some reason. Any idea why that would be the case @natebosch @jakemac53 ?

For the short term, we could revert the aforementioned PR to unblock the engine tree.

@Hixie
Copy link
Contributor

Hixie commented Oct 7, 2021

Weird, why didn't this fail pre-commit?

@mdebbar
Copy link
Contributor

mdebbar commented Oct 7, 2021

Probably because we only call markTestSkipped when running the tests on the engine repo? When the tests run in the flutter repo, they will do the actual gold comparisons. But on the engine repo, it uses FlutterSkippingFileComparator, which calls markTestSkipped.

@Hixie
Copy link
Contributor

Hixie commented Oct 7, 2021

Oh I see, I didn't realise the failure was on the engine side, my bad.

@natebosch
Copy link
Contributor

natebosch commented Oct 7, 2021

which fails on web because Invoker.current is null for some reason.

The calls aren't added directly in test files in that PR, so it's hard to tell whether they are in utilities exclusively used in tests. markTestSkipped won't work unless you are specifically executing within a test run by the test runner.

Can you tell me more about the context where this is running? I do see some test case output - is the markTestSkipped call a part of that test case? Or is this a test where it's invoking a tool to separately compile and run an app?

The stack trace contains main.<anonymous closure> (file:///b/s/w/ir/x/t/flutter_tools.JCITJV/flutter_web_platform.QNOXQB/listenerPZNWUU/listener.dart:29:53) - any chance I can see what that file looked like?

@Hixie
Copy link
Contributor

Hixie commented Oct 7, 2021

It's called from within a test expectation that's called in an expect that's called in a test. (edit: or, rather, that's how it's intended to be used. I guess there is an outside chance that we're calling it incorrectly here?)

The listener.dart file is autogenerated by the flutter tool. You can see the code that generates it here: https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/test/flutter_platform.dart#L579

@Hixie
Copy link
Contributor

Hixie commented Oct 7, 2021

Oh maybe actually this is the code?
https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/test/flutter_web_goldens.dart#L172
That does seem a bit dubious...

@natebosch
Copy link
Contributor

The listener.dart file is autogenerated by the flutter tool. You can see the code that generates it here: https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/test/flutter_platform.dart#L579

Yeah I was hoping to see the specific file in a reproduction case. I took a look at the generator and the line numbers vary based on a bunch of conditionals.

@Hixie
Copy link
Contributor

Hixie commented Oct 7, 2021

Oh, I see the problem. This is indeed being called from within a test -- but the way it works is the comparator then communicates with a separate Dart program running in its own context which doesn't know it's in a test, so markTestSkipped can't work there.

@natebosch
Copy link
Contributor

a separate Dart program running in its own context

Yeah, markTestSkipped can't be used there.

Unfortunately there really isn't a way to check any of this statically. We could make a more explicit error message though. Is there any particular phrasing that might have helped here?

@Hixie
Copy link
Contributor

Hixie commented Oct 7, 2021

"markTestSkipped was called while no test was active" or something, maybe?
I'll follow up on this side in terms of figuring out how to smuggle the skip information to the right place...

@mdebbar
Copy link
Contributor

mdebbar commented Oct 7, 2021

Is there any particular phrasing that might have helped here?

Maybe something like this: dart-lang/test#1533 ?

@mdebbar
Copy link
Contributor

mdebbar commented Oct 7, 2021

I'll follow up on this side in terms of figuring out how to smuggle the skip information to the right place...

@Hixie would you mind if I assign this issue to you? And since the engine tree is back to green, I don't think this issue is a P0 anymore. I'll make it a P3 but feel free to update.

@mdebbar mdebbar removed their assignment Oct 7, 2021
@mdebbar mdebbar added P1 High-priority issues at the top of the work list a: tests "flutter test", flutter_test, or one of our tests and removed P0 Critical issues such as a build break or regression labels Oct 7, 2021
@Hixie
Copy link
Contributor

Hixie commented Oct 7, 2021

Yup

@Hixie Hixie self-assigned this Oct 7, 2021
natebosch added a commit to dart-lang/test that referenced this issue Oct 7, 2021
See flutter/flutter#91432

Pull out a `_currentInvoker` utility to share the exception fallback
behavior between `printOnFailure` and `markTestSkipped`.
@Hixie Hixie closed this as completed Oct 8, 2021
natebosch added a commit to dart-lang/test that referenced this issue Oct 13, 2021
See flutter/flutter#91432

Pull out a `_currentInvoker` utility to share the exception fallback
behavior between `printOnFailure` and `markTestSkipped`.
@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a: tests "flutter test", flutter_test, or one of our tests P1 High-priority issues at the top of the work list platform-web Web applications specifically
Projects
None yet
Development

No branches or pull requests

4 participants