properly pass on gradle exit code (#71484)

[mx1up]

Description

the gradle process's exit code variable was not properly referenced. That way, when the flutter build failed, it did not fail the gradle task. see #71484 for more info

Related Issues

fix #71484
fix #74479

Tests

I added the following tests:

I have not written any test yet as I don't have any clue where to start. Please provide some pointer/example?

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.

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I signed the CLA.
• I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I updated/added relevant documentation (doc comments with ///).
• All existing and new tests are passing.
• The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
• I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

I ran all tests using dev channel and master channel flutter, but a lot of tests failed. The errors don't seem related to my change though. How to properly run the tests?

flutter analyze --flutter-repo also reports lots of errors like this:

  error - The function 'Offset' isn't defined - ..\..\..\..\programs\flutter\packages\flutter\lib\src\material\animated_icons\data\event_add.g.dart:95:13 - undefined_function
  error - The values in a const list literal must be constants - ..\..\..\..\programs\flutter\packages\flutter\lib\src\material\animated_icons\data\event_add.g.dart:95:13 - non_constant_list_element
  error - Const variables must be initialized with a constant value - ..\..\..\..\programs\flutter\packages\flutter\lib\src\material\animated_icons\data\event_add.g.dart:96:13 - const_initialized_with_non_constant_value
  error - Invalid constant value - ..\..\..\..\programs\flutter\packages\flutter\lib\src\material\animated_icons\data\event_add.g.dart:96:13 - invalid_constant

• No, no existing tests failed, so this is not a breaking change.
• Yes, this is a breaking change. If not, delete the remainder of this section.
  • I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
  • I got input from the developer relations team, specifically from: Replace with the names of who gave advice
  • I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

[flutter-dashboard]

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.

[mx1up]

I believe there is a similar bug at

flutter/packages/flutter_tools/lib/src/android/gradle.dart

Line 680 in 2598610

exitCode: exitCode,

should I include a fix as well in the PR or create a separate issue/PR ?

[xster]

oh, awesome find! Thanks for your PR.

For tests, generally look for usages of the public method or of the top level command being used in existing tests are use them as inspiration :)

For instance, you can add one test method in https://github.com/flutter/flutter/blob/master/packages/flutter_tools/test/general.shard/android/gradle_test.dart#L1635. Assemble together a skeleton project and make the dart file bogus, assert that the status code is sad.

Then in https://github.com/flutter/flutter/blob/master/packages/flutter_tools/test/commands.shard/permeable/build_aar_test.dart (which tests at a higher level), use the same helpers to create a project, break the dart file, then invoke the build like the other tests and assert the throw or exit code.

[blasten]

good catch. For the test, you could look at this line

flutter/packages/flutter_tools/test/general.shard/android/gradle_test.dart

Line 1665 in 2520d53

)).thenAnswer((_) async => ProcessResult(1, 0, '', ''));

, and replace the second parameter (the exit code) for != 0.

[mx1up]

thanks for the tips! i will have a look next week

[flutter-dashboard]

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[jonahwilliams]

@mx1up are you still interested in working on this? If so, you'll need to pull in the latest master to get the tests working again

[mx1up]

@jonahwilliams yes, I will have a look at it next week. I have just returned from a month long holiday, that explains the lack of follow-up.

[mx1up]

I had a look at the code, but have a few questions.

For instance, you can add one test method in https://github.com/flutter/flutter/blob/master/packages/flutter_tools/test/general.shard/android/gradle_test.dart#L1635. Assemble together a skeleton project and make the dart file bogus, assert that the status code is sad.

did you reference a random line in that file, or do you really mean I should add an override there (for my new test then). or do you simply mean, add a test next to (at the same level as) testUsingContext('indicates that an APK has been built successfully',

Also, is my test atypical in the sense that it really needs to build and not mock everything out? I did not find any similar test in that file? Is that correct? If so, it makes it really hard to get started..

Then in https://github.com/flutter/flutter/blob/master/packages/flutter_tools/test/commands.shard/permeable/build_aar_test.dart (which tests at a higher level), use the same helpers to create a project, break the dart file, then invoke the build like the other tests and assert the throw or exit code.

here I did find tests that create a sample project like this:
final String projectPath = await createProject(tempDir, arguments: <String>['--no-pub', '--template=module']);
should I use a similar approach but for the gradle_test.dart test?

I did try adapting a test here like this (obviously it is only a minor change to try out, just overwriting the main.dart file):

      testUsingContext('fails if flutter build fails', () async {
        final String projectPath = await createProject(tempDir,
            arguments: <String>['--no-pub', '--template=module']);

        final dartio.File libMainFile = dartio.File(globals.fs.path.join(projectPath, 'lib', 'main.dart'));
        libMainFile.createSync(recursive: true);
        libMainFile.writeAsStringSync('''
        void main() => shibby();
        '''
        );

        await expectLater(() async {
          await runBuildAarCommand(
            projectPath,
            // arguments: <String>['--no-pub'],
          );
        },
            throwsToolExit(
              message:
                  'No Android SDK found. Try setting the ANDROID_SDK_ROOT environment variable',
            ));
      }, overrides: <Type, Generator>{
        AndroidSdk: () => mockAndroidSdk,
        FlutterProjectFactory: () => FakeFlutterProjectFactory(tempDir),
        ProcessManager: () => mockProcessManager,
      });

but it failed in another way:

Expected: throws (<Instance of 'ToolExit'> and satisfies function)
  Actual: <Closure: () => Future<Null>>
   Which: threw ToolExit:<Exception: Gradle task assembleAarDebug failed to produce LocalDirectory: '/tmp/flutter_tools.BSSSCO/flutter_tools_packages_test.AZFECY/flutter_project/build/host/outputs/repo'.>

I'm afraid I'm kinda stuck, any extra info/explanation much appreciated

[jonahwilliams]

@blasten could you take a look and give @mx1up some advice when you have the time?

[pedromassango]

This PR will also fix #74479. Can anyone also metion it in OP so that it will be fixed once this is merged?

[jonahwilliams]

@zanderso

[littleGnAl]

Any update on this?

[zanderso]

@mx1up Copy and paste the test that @blasten referenced. The one starting here:

flutter/packages/flutter_tools/test/general.shard/android/gradle_test.dart

Line 1642 in 2520d53

testUsingContext("doesn't indicate how to consume an AAR when printHowToConsumeAaar is false", () async {

.

Change the second parameter to ProcessResult() to be non-zero to trigger the code that you modified.

Change the test to expect a tool exit from the call to buildGradleAar() by wrapping it in a try {} catch {}, then inspect the exitCode on the exception object and check that it is correct.

[littleGnAl]

Hello @mx1up
According to the references of @zanderso and @blasten, I think the test case below should be right

testUsingContext("doesn't indicate how to consume an AAR when flutter build failure", () async {
  final File manifestFile = fileSystem.file('pubspec.yaml');
  manifestFile.createSync(recursive: true);
  manifestFile.writeAsStringSync('''
    flutter:
      module:
        androidPackage: com.example.test
    '''
  );

  fileSystem.file('.android/gradlew').createSync(recursive: true);

  fileSystem.file('.android/gradle.properties')
    .writeAsStringSync('irrelevant');

  fileSystem.file('.android/build.gradle')
    .createSync(recursive: true);

  // Let any process start. Assert after.
  when(mockProcessManager.run(
    any,
    environment: anyNamed('environment'),
    workingDirectory: anyNamed('workingDirectory'),
  )).thenAnswer((_) async => ProcessResult(1, 1, '', ''));

  fileSystem.directory('build/outputs/repo').createSync(recursive: true);

  await expectLater(() async {
    await buildGradleAar(
      androidBuildInfo: const AndroidBuildInfo(BuildInfo(BuildMode.release, null, treeShakeIcons: false)),
      project: FlutterProject.current(),
      outputDirectory: fileSystem.directory('build/'),
      target: '',
      buildNumber: '1.0',
    );
  }, 
  throwsToolExit(
    exitCode: 1, 
    message: 'Gradle task assembleAarRelease failed with exit code 1.'),
  );

  expect(
    testLogger.statusText.contains('Consuming the Module'),
    isFalse,
  );
}, overrides: <Type, Generator>{
  AndroidSdk: () => mockAndroidSdk,
  AndroidStudio: () => mockAndroidStudio,
  Cache: () => cache,
  Platform: () => android,
  FileSystem: () => fileSystem,
  ProcessManager: () => mockProcessManager,
});

Can you please try to copy the test case, and see the CI passed or not? Hope this PR can be merged ASAP.

[mx1up]

@zanderso thank you very much for the pointers. I think I managed to do it and afterwards I could compare my results with @littleGnAl (also thanks :) ). The only thing I was not sure about in @littleGnAl 's solution, if it is really necessary to include the "consuming the module" test? If the expection is thrown, the test succeeds, is that not sufficient? I included it anyway because i assume @littleGnAl has more experience than me ;)

@xster mentioned another test to be added in build_aar_test.dart . I'll have another look if I can produce a test there. I am currently looking to adapt this test:

flutter/packages/flutter_tools/test/commands.shard/permeable/build_aar_test.dart

Line 259 in 021311e

testUsingContext('throws throwsToolExit if AndroidSdk is null', () async {

Would that be a good starting point?

I will continue tomorrow.

[blasten]

LGTM + nit suggestion

[mx1up]

if another more high level test is needed, I gave it a try in another branch in order not to unnecessarily pollute this PR:
mx1up@c9afe50
This test mocks the processManager too.

In another attempt (not committed), I tried failing the build doing a real build and damaging the main.dart like @xster suggested but then I got an error message like this:

The project name '.android' must not start or end with a '.'. Set the 'rootProject.name' or adjust the 'include' statement ...cut

the code looked something like this:

    group('build aar', () {
      testUsingContext('throws throwsToolExit if build command fails', () async {
        final String projectPath = await createProject(tempDir, arguments: <String>['--no-pub', '--template=module']);

        // break dart compilation
        final File mainDartFile = fileSystem.file(projectPath + '/lib/main.dart');
        await mainDartFile.writeAsString('does not compile');

        await expectLater(() async {
          await runBuildAarCommand(
            projectPath,
            arguments: <String>['--no-pub'],
          );
        },
            throwsToolExit(
              exitCode: 1,
            ));
      }, overrides: <Type, Generator>{
//real build, do not override
        // AndroidSdk: () => mockAndroidSdk,
        // FlutterProjectFactory: () => FakeFlutterProjectFactory(tempDir),
        // ProcessManager: () => mockProcessManager,
      });
    });

[mx1up]

thanks for merging. btw, i believe passing the exitcode (with same bug, should be result.exitCode) on this line is useless:

flutter/packages/flutter_tools/lib/src/android/gradle.dart

Line 658 in cc95c97

exitCode: exitCode,

as the exitCode at that point will always be 0. However, that means the build command should have succeeded and the directory should exist..