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

Add Super Editor back to test registry #172

Merged
merged 3 commits into from
Nov 11, 2022

Conversation

matthew-carroll
Copy link
Contributor

Super Editor was removed from the test registry for not keeping up with Flutter's breaking changes.

This PR adds Super Editor back to the test registry after adjusting to the latest breaking changes.

@matthew-carroll
Copy link
Contributor Author

matthew-carroll commented Nov 9, 2022

@Hixie I'm not sure why CI is failing. The reported failure is:

| test/super_textfield/super_textfield_auto_scroll_test.dart:175:13: Error: Final field 'frameCount' is not initialized.
| Try to initialize the field in the declaration or in every constructor.
|   final int frameCount;
|             ^^^^^^^^^^

Except that test does compile, and it runs, and it passes (on my end).

This is the source: https://github.com/superlistapp/super_editor/blob/main/super_editor/test/super_textfield/super_textfield_auto_scroll_test.dart#L177

@dnfield
Copy link
Contributor

dnfield commented Nov 10, 2022

The test harness is running dart fix --apply.

That sees that the constructor parameter here never gets set to anything else, and removes it: https://github.com/superlistapp/super_editor/blob/6a90a3def633161183857a973ba251eb46176da3/super_editor/test/super_textfield/super_textfield_auto_scroll_test.dart#L171

But with that removed your variable is uninitialized. This seems like a bug in dartfix (/cc @bwilkerson and maybe @Piinks?).

In the mean time, you could work around this by removing the parameter from your constructor.

@dnfield
Copy link
Contributor

dnfield commented Nov 10, 2022

if you look at analysis with Flutter master, that parameter gets some blue squigglies saying

A value for optional parameter 'frameCount' isn't ever given.
Try removing the unused parameter.dart[unused_element](https://dart.dev/diagnostics/unused_element)

@Hixie
Copy link
Contributor

Hixie commented Nov 10, 2022

Oh interesting; we should probably run with less drastic lints? (IIRC dart fix only applies fixes for active lints?)

@matthew-carroll
Copy link
Contributor Author

If I add an annotation to ignore the complaint, will this CI respect that?

@dnfield
Copy link
Contributor

dnfield commented Nov 10, 2022

Oh interesting; we should probably run with less drastic lints? (IIRC dart fix only applies fixes for active lints?)

If we can exclude the unused_element lint until dart fix handles this better we should do that.

@dnfield
Copy link
Contributor

dnfield commented Nov 10, 2022

If I add an annotation to ignore the complaint, will this CI respect that?

I'm not sure, you could probably test it locally by adding the annotation and then running dart fix --apply and seeing what it does though.

@dnfield
Copy link
Contributor

dnfield commented Nov 10, 2022

(I just tested locally with an // ignore: unused_element and it seems to preserve the parameter now)

@matthew-carroll
Copy link
Contributor Author

I'm gonna clean up all the Dart Analysis complaints. I need to do some PR and branch stuff on my end before I bring that over here.

@matthew-carroll
Copy link
Contributor Author

@Hixie it looks like tests are timing out again

@dnfield
Copy link
Contributor

dnfield commented Nov 10, 2022

The Windows failures appear to be in a different package and related to a different issue in Skia that happened yesterday. You'll probably have to merge up to head of this repo to pick up fixes that happened there.

@dnfield
Copy link
Contributor

dnfield commented Nov 10, 2022

(Skia changed a sampling algorithm in the CPU backend which altered some goldens in the failing tests)

@Piinks
Copy link
Contributor

Piinks commented Nov 10, 2022

This seems like a bug in dartfix

That definitely seems like a bug, not from the fix rules defined in flutter/flutter fix data though. I am not sure where that transformation is coming from.

IIRC dart fix only applies fixes for active lints

This is true!

@matthew-carroll
Copy link
Contributor Author

@dnfield can you paste the error that you're seeing? I just scrolled through one of the Windows logs and didn't see any failures in super_editor tests. I also upgrade Flutter locally and ran all super_editor tests ands nothing failed.

@dnfield
Copy link
Contributor

dnfield commented Nov 11, 2022

@dnfield can you paste the error that you're seeing? I just scrolled through one of the Windows logs and didn't see any failures in super_editor tests. I also upgrade Flutter locally and ran all super_editor tests ands nothing failed.

 00:22 +9: orientation/isometric
| ══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
| The following assertion was thrown while running async test code:
| Golden "test-maps/orientation/isometric_windows.png": Pixel test failed, 3.59% diff detected.
| Failure feedback can be found at
| /C:/Users/ContainerAdministrator/AppData/Local/Temp/flutter_customer_testing.DanTup_tiler.f458fa29/tests/test/failures
|
| When the exception was thrown, this was the stack:
| #0      LocalFileComparator.compare (package:flutter_test/src/_goldens_io.dart:101:7)
| <asynchronous suspension>
| <asynchronous suspension>
| (elided one frame from package:stack_trace)
|
| The exception was caught asynchronously.
| ════════════════════════════════════════════════════════════════════════════════════════════════════
|
| 00:22 +9 -1: orientation/isometric [E]
|   Test failed. See exception logs above.
|   The test description was: orientation/isometric
|
|
| To run this test again: C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\flutter\bin\cache\dart-sdk\bin\dart.exe test C:\Users\ContainerAdministrator\AppData\Local\Temp\flutter_customer_testing.DanTup_tiler.f458fa29\tests\test\render_test.dart -p vm --plain-name 'orientation/isometric'
|
| 00:22 +9 -1: geometry/orthogonal
| 00:22 +10 -1: geometry/orthogonal
| 00:22 +10 -1: geometry/isometric
| 00:22 +10 -1: geometry/isometric
| ══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
| The following assertion was thrown while running async test code:
| Golden "test-maps/geometry/isometric_windows.png": Pixel test failed, 3.05% diff detected.
| Failure feedback can be found at
| /C:/Users/ContainerAdministrator/AppData/Local/Temp/flutter_customer_testing.DanTup_tiler.f458fa29/tests/test/failures
|
| When the exception was thrown, this was the stack:
| #0      LocalFileComparator.compare (package:flutter_test/src/_goldens_io.dart:101:7)
| <asynchronous suspension>
| <asynchronous suspension>
| (elided one frame from package:stack_trace)
|
| The exception was caught asynchronously.
| ════════════════════════════════════════════════════════════════════════════════════════════════════
|
| 00:22 +10 -2: geometry/isometric [E]
|   Test failed. See exception logs above.
|   The test description was: geometry/isometric
|
|
| To run this test again: C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\flutter\bin\cache\dart-sdk\bin\dart.exe test C:\Users\ContainerAdministrator\AppData\Local\Temp\flutter_customer_testing.DanTup_tiler.f458fa29\tests\test\render_test.dart -p vm --plain-name 'geometry/isometric'

I think if you merge this repo up to head you'll be in the green.

@dnfield
Copy link
Contributor

dnfield commented Nov 11, 2022

To be clear: the failures are not coming from super_editor, they're coming from a different package that had to be patched yesterday.

@matthew-carroll
Copy link
Contributor Author

Oh, you mean "this repo" as in the test registry, not "this repo" as in some other repo that caused a test failure. Ok. I'll rebase my test registry.

@matthew-carroll matthew-carroll force-pushed the add-super-editor-back-to-test-registry branch from 73022bf to b6687b7 Compare November 11, 2022 00:26
Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@auto-submit auto-submit bot merged commit e01991c into main Nov 11, 2022
@dnfield dnfield deleted the add-super-editor-back-to-test-registry branch November 11, 2022 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants