Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@nturgut
Copy link
Contributor

@nturgut nturgut commented Apr 9, 2020

Changes to run integration tests ion Cirrus using felt tool.

  • fetch flutter repo
  • flutter pub get from the fetched framework.
  • enable cirrus tests
  • added a lag use-system-flutter if the local developer wants to use the system version.

Fixes some tasks in the issue: flutter/flutter#52983

@auto-assign auto-assign bot requested a review from liyuqian April 9, 2020 00:58
@nturgut nturgut force-pushed the run_int_tests_on_ci branch from 78ead30 to 4e156be Compare April 9, 2020 00:58
@nturgut nturgut requested review from yjbanov and removed request for liyuqian April 9, 2020 00:58
@nturgut nturgut force-pushed the run_int_tests_on_ci branch from 6d6320b to c3ff9e2 Compare April 9, 2020 17:26
@yjbanov yjbanov changed the title Run int tests on ci Run integration tests on ci Apr 9, 2020
Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

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

Mostly nits. My only major concern is that we can end up with two versions of the engine and two versions of the framework as part of any given build. Which exact version is used depends on the environment we are running in. I'm worried that this will lead to unpredictable and non-reproducible build breakages.

@nturgut nturgut requested a review from yjbanov April 9, 2020 19:17
/// commit.
///
/// Use engine/src/flutter/.dart_tools to clone the Flutter repo.
/// TODO(nurhan): Use git pull instead if repo exists.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@nturgut nturgut merged commit deef266 into flutter:master Apr 13, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 13, 2020
goderbauer pushed a commit to goderbauer/engine that referenced this pull request Apr 16, 2020
* changing felt script to fetch flutter

* changing the clone_flutter.sh code

* running integration tests with felt on cirrus. fetch framework in CI (not in local).

* only run cirrus tests on chrome. fix a comma in the flutter command path

* adding comments to public flags

* use local engine parameter for flutter pub get

* change flutter executable used for flutter drive command

* fix a cleanup issue. address comments. add toolException. enable web in flutter

* address reviwer comments. fix issue with local-engine

* address reviwer comments. fix issue with local-engine

* using engine/flutter/.dart_tools as clone directory. enabling clone script for local usage

* clean flutter repo with felt clean. add a flag to skip cloning the repo. always clone the repo even for local development, unless this flag is set

* fixing typos. updating readme for the new flag.

* fix directory error

* addressing reviewer comments
io.Directory get engineDartToolDir => io.Directory(pathlib.join(
engineSrcDir.path,
'flutter',
'.dart_tool',
Copy link

Choose a reason for hiding this comment

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

This seems useful for android and iOS e2e too. We currently have duplicated scripts to build AOT/JIT apps, but it would be a win if we can just reuse the tool logic.

/// Path to the ".dart_tool" directory living under `engine/src/flutter`.
///
/// This is a designated area for tool downloads which can be used by
/// multiple platforms. For exampe: Flutter repo for e2e tests.
Copy link

Choose a reason for hiding this comment

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

nit: example

import 'package:web_driver_installer/chrome_driver_installer.dart';

import 'chrome_installer.dart';
import 'common.dart';

Choose a reason for hiding this comment

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

Yes

import 'chrome_installer.dart';
import 'common.dart';
import 'environment.dart';
import 'exceptions.dart';

Choose a reason for hiding this comment

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

  • [ ]
Suggested change
import 'exceptions.dart';
import 'exceptions.dart';```suggestion
import 'exceptions.dart';

Choose a reason for hiding this comment

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

Yes

/// tests shutdown.
final io.Directory _drivers;

IntegrationTestsManager(this._browser)

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

// TODO(nurhan): https://github.com/flutter/flutter/issues/52987
return await _runTests();
}
}

Choose a reason for hiding this comment

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

I

await cleanup();
exitCode = 1;
}
} on UsageException catch (e) {

Choose a reason for hiding this comment

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

__

Choose a reason for hiding this comment

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

😊

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants