Skip to content

Conversation

@stuartmorgan-g
Copy link
Collaborator

An update to args changed the failure mode for a missing flag; this updates the tests accordingly to fix the tree.

An update to `args` changed the failure mode for a missing flag; this
updates the tests accordingly to fix the tree.
Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

What is different between --changelog and --version?

How come this test is not failing in a similar fashion? fails if --version is missing (L78)

@ditman
Copy link
Member

ditman commented Jun 5, 2023

Replying to myself, here's the relevant diff:

- If an option is `mandatory` but not provided, the parser throws an
- [`ArgParserException`][ArgParserException].
+ If an option is `mandatory` but not provided, the results object throws an
+ [`ArgumentError`][ArgumentError] on retrieval.

(From this change)

(According to pub, package:args got published "2 hours ago", which matches the time of the first change that failed in flutter/packages)

@stuartmorgan-g
Copy link
Collaborator Author

What is different between --changelog and --version?

How come this test is not failing in a similar fashion? fails if --version is missing (L78)

Ah, good catch. It turns out that test is subtly wrong, because changelog is blank, and that's also a failure case. So the test is failing with a UsageException thrown by our code because changelog is blank and that's evidently happening before the ArgumentError. I'll add a fix to that test.

@PROGrand
Copy link
Contributor

PROGrand commented Jun 5, 2023

Linux/Window repo_tools_tests fails with args: 2.4.2. Is CI engine uses tools different from flutter/packages main?
but flutter/packages are bundled with tools depending on args: ^2.1.0 and sdk: '>=2.14.0 <4.0.0'

So i suggest for universal:

      Error? commandError;
      Exception? commandException;
      await runCapturingPrint(runner, <String>[
        'update-release-info',
        '--version=next',
      ], errorHandler: (Error e) {
        commandError = e;
      }, exceptionHandler: (Exception e) {
        commandException = e;
      });

      expect(
        commandError is ArgumentError || commandException is UsageException,
        isTrue,
      );

@ditman
Copy link
Member

ditman commented Jun 5, 2023

flutter/packages are bundled with tools depending on args: ^2.1.0

Can we update the dependency to args: ^2.4.2?

@stuartmorgan-g
Copy link
Collaborator Author

flutter/packages are bundled with tools depending on args: ^2.1.0

Can we update the dependency to args: ^2.4.2?

I considered that when writing the PR, but it requires Dart 2.19, so we'd have to drop the 3.3 legacy tests. I was just going to ignore this since we unit test the tools on master.

@stuartmorgan-g
Copy link
Collaborator Author

Linux/Window repo_tools_tests fails with args: 2.4.2.

That is what this PR fixes, as discussed in previous comments.

Is CI engine uses tools different from flutter/packages main?

I don't understand the question.

So i suggest for universal

Why? Who is running the flutter/packages repo tools' unit tests with Flutter 3.3 or earlier?

@PROGrand
Copy link
Contributor

PROGrand commented Jun 5, 2023

I have my PR and i trying to deal with tool/test.

  1. Error log shows arg_results.dart 70:7 - it is from args: 2.4.2, but in my PR there is unchanged args: ^2.1.0
image
  1. Also it show source of expect error as test/update_release_info_command_test.dart 54

  2. But there is my PR peace of tools, and there is no significal code at line 54:

image

So, CI runs not PR's tools, and checks no PR's code.

@stuartmorgan-g
Copy link
Collaborator Author

LUCI had some kind of meltdown so everything is red, but I re-ran the repo_tools_tests runs and they are green. Since this is only tool unit test, and thus incapable of affecting anything else, landing with red tests to fix the tree.

@stuartmorgan-g stuartmorgan-g merged commit 08374a5 into flutter:main Jun 5, 2023
@stuartmorgan-g stuartmorgan-g deleted the arg-change-oob-fix branch June 5, 2023 20:20
@stuartmorgan-g
Copy link
Collaborator Author

I have my PR and i trying to deal with tool/test.

The only way to deal with out-of-band failures is to wait for them to be fixed (as has now happened), then rebase/merge the latest main.

  1. Error log shows arg_results.dart 70:7 - it is from args: 2.4.2, but in my PR there is unchanged args: ^2.1.0

Yes, ^2.1.0 includes 2.4.2 in its range.

So, CI runs not PR's tools, and checks no PR's code.

I don't understand how you reached that conclusion, but it is not correct.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 6, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 6, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 6, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 6, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jun 6, 2023
flutter/packages@db4e5c2...da72219

2023-06-06 42692766+dimabran@users.noreply.github.com remove unnecessary typed_data imports (flutter/packages#4136)
2023-06-06 43054281+camsim99@users.noreply.github.com [local_auth_android] Bump androidx.fragment to 1.5.7 and androidx.core to 1.10.1 (flutter/packages#4142)
2023-06-05 ditman@gmail.com [google_maps] Endorses package:google_maps_flutter_web. (flutter/packages#4124)
2023-06-05 ditman@gmail.com [ci] Removes bespoke web scripts. (flutter/packages#4129)
2023-06-05 arteevraina@gmail.com [webview_flutter]: fix typo (flutter/packages#4070)
2023-06-05 49699333+dependabot[bot]@users.noreply.github.com [camera]: Bump com.google.guava:guava from 31.1-android to 32.0.0-android in /packages/camera/camera_android_camerax/android (flutter/packages#4116)
2023-06-05 stuartmorgan@google.com [tools] Fix OOB test error (flutter/packages#4144)
2023-06-05 46547604+dsambuk@users.noreply.github.com [path_provider] Allow win32 up to version 5.x (flutter/packages#4125)
2023-06-05 49699333+dependabot[bot]@users.noreply.github.com [sign_in]: Bump com.google.guava:guava from 31.1-android to 32.0.0-android in /packages/google_sign_in/google_sign_in_android/android (flutter/packages#4112)
2023-06-05 49699333+dependabot[bot]@users.noreply.github.com [video_player]: Bump exoplayer_version from 2.18.6 to 2.18.7 in /packages/video_player/video_player_android/android (flutter/packages#4053)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC flutter-ecosystem@google.com,rmistry@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants