Fix: remove cross-import in scaffold_test.dart#183790
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request resolves a cross-import in scaffold_test.dart by duplicating semantics_tester.dart. While this achieves test isolation, it introduces a large duplicated file, which can be a maintenance concern. I've also found a couple of potential pre-existing bugs in the copied semantics_tester.dart file and have left comments with suggestions to fix them. It would be great to fix them here to avoid propagating them. Also, consider if there's a way to share this test utility without duplication to improve long-term maintainability.
navaronbracke
left a comment
There was a problem hiding this comment.
This is probably not the right change to make.
The semantics tester tooling should probably be in flutter_test, if it isn't already.
Besides that, the OP lists an issue as fixed, but that is not the case, since this PR should use "Part of" instead of "Fixes". There's probably more usages that need to be cleaned up.
|
TestSemantics is not something we want to make public since it assert the entire semantics tree and can generate a lot of noise. We should move away from this matcher. I vote for duplicating the code in material for now and file an issue to clean up existing tests. |
|
@aburiro Are you still available to get this PR ready to land? See my comment above. |
As decided in #183790 (comment), we should avoid using TestSemantics. Let's communicate that for now until we can remove the class entirely. Part of #184367. --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
As decided in flutter#183790 (comment), we should avoid using TestSemantics. Let's communicate that for now until we can remove the class entirely. Part of flutter#184367. --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
This pull request is being closed because it has not been updated in the last 21 days after a request for more information. If you are still working on this, please feel free to reopen it or file a new pull request. Thanks for your contribution. |
FixesPart of #182636This PR removes a cross-import in
scaffold_test.dartby duplicatingsemantics_tester.dartinto the material test directory.This ensures tests do not depend on utilities from other libraries, aligning with the test isolation guidelines.