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 support for Dart-side group()s and multiple test()s #1619

Closed
Tracked by #1501
bartekpacia opened this issue Aug 9, 2023 · 19 comments · Fixed by #1634
Closed
Tracked by #1501

Add support for Dart-side group()s and multiple test()s #1619

bartekpacia opened this issue Aug 9, 2023 · 19 comments · Fixed by #1634
Labels
feature New feature request package: patrol Related to the patrol package (native automation, test bundling)

Comments

@bartekpacia
Copy link
Contributor

bartekpacia commented Aug 9, 2023

A subtask of #1501.

@bartekpacia
Copy link
Contributor Author

bartekpacia commented Aug 9, 2023

A problem with this is that the execution model of tests will change.

Developers usually expect that their test file executes once, roughly from top to bottom. But when we implement the feature that this issue talks about, it'll become more complex.

Example

Let's consider a test file:

void main() {
  late int photoCount;

  group('test that photos are downloaded', () {
    test('download photos', () {
      photoCount = ...;
    });

    test('tap on the last but one photo', () {
      // do something that depends on photoCount
    });
  });
}

The user might reasonably expect that photoCount will be set during the second test, because it's set by the previous test. But it'll not be set, because every test case would be run in a new app process.

@bartekpacia bartekpacia changed the title Add support for Dart-side group()s Add support for Dart-side group()s and multiple test()s Aug 9, 2023
@shilangyu
Copy link
Contributor

Maybe I am not speaking for the majority here, but in the example you gave I think it would be totally reasonable for it to fail. Relying on execution order of individual tests sounds like an anti pattern to me. Test tooling seems to even support options that would break dependency on order (--test-randomize-ordering-seed, and maybe --concurrency/--total-shards?)

@bartekpacia
Copy link
Contributor Author

Relying on execution order of individual tests sounds like an anti pattern to me.

It is, but I've seen our QAs do it quite a few times. I'm afraid that those additional restrictions will be tricky for them.

@bartekpacia
Copy link
Contributor Author

bartekpacia commented Aug 9, 2023

We could also run a whole test file in a new app process, instead of running each test case in a new app process. This would maybe feel more natural?

I think it'd also make the distinction between the two types of "grouping" more clear:

  • grouping by filesystem hierarchy - each file runs in a new process
  • grouping by package:test hierarchy in individual file - all groups and tests run in the same process

@bartekpacia
Copy link
Contributor Author

Idea: lint to not create global state ("non-local-to-test-case state") in Patrol tests.

@bartekpacia
Copy link
Contributor Author

bartekpacia commented Aug 10, 2023

Also, a big consideration is patrol develop. It must continue to work.

I propose the following:

  • the default behavior stays the same. patrol develop -t file executes whole file.
  • we add --group and --test options to patrol develop, to allow for more granular selection of test(s) to execute. Example: patrol develop -t file --group 'smoke tests' --test '(first)'

@bartekpacia bartekpacia added feature New feature request package: patrol Related to the patrol package (native automation, test bundling) labels Aug 11, 2023
@bartekpacia
Copy link
Contributor Author

Test names should be the same on iOS and Android.

Currently, native test name is: file path + groups + test case name.

The problem is with encoding this in Swift/Objective-C. We can't have names like on Android:

Screenshot 2023-08-14 at 1 36 05 PM

Related issue: #1486

@bartekpacia
Copy link
Contributor Author

bartekpacia commented Aug 14, 2023

Test names proposal

Restrictions:

An example

It's a good practice to make group()s and test()s read like a sentence:

// integration_test/examples/some_example_test.dart

group('HomeScreen', () {
  group('when not signed in', () {
    test('shows sign-in dialog'), () {
      // ...
     });
    
    test('does something else'), () {
      // ...
     });
 
  });

  group('when signed in'), () {
    test('shows newsletter dialog'), () {
      // ..
    });

    test('does a nice thing'), () {
      // ..
    });
});

It'd result in 4 test cases:

examples__some_example__HomeScreenWhenNotSignedInShowsSignInDialog
examples__some_example__HomeScreenWhenNotSignedInDoesSomethingElse
examples__some_example__HomeScreenWhenSignedInShowsNewsletterDialog
examples__some_example__HomeScreenWhenSignedInDoesANiceThing

@fylyppo
Copy link
Collaborator

fylyppo commented Aug 14, 2023

Because of the restrictions this proposal it's ok but I don't like these restrictions at all. It makes it ugly and for my test cases it'll be unreadable. My example test case name: "Verify navigation from BottomNavigationBarContainer when accessory is not connected".
So in my case maybe better solution is to separate words with underscore

@bartekpacia
Copy link
Contributor Author

bartekpacia commented Aug 14, 2023

We could do:

examples__some_example__HomeScreen_WhenNotSignedIn_ShowsSignInDialog

Explainer: we replace filesystem / with a double underscore __, and concatenate group()s and test()s with a single underscore _. We also trim the trailing _test.dart from the filename, because it's redundant (so some_example_test.dart -> some_example).

@fylyppo
Copy link
Collaborator

fylyppo commented Aug 14, 2023

okay, it's better idea for me

@bartekpacia
Copy link
Contributor Author

This will require us to prohibit using _ in group()s and test()s.

@ashwinichannaveerappa
Copy link

ashwinichannaveerappa commented Aug 29, 2023

@bartekpacia is group() and test() feature is already available? I tried using it, but its not working the app gets stuck at login screen

group("Verify that user..", () { patrolTest("Test 1", ($) async { await loadLocalization();await baseClass.initializeApp();await baseClass.loginWithValidCredentials($); patrolTest(" Test 2",($) async {});});

@mkhtradm01
Copy link

mkhtradm01 commented Sep 6, 2023

@bartekpacia is group() and test() feature is already available? I tried using it, but its not working the app gets stuck at login screen

group("Verify that user..", () { patrolTest("Test 1", ($) async { await loadLocalization();await baseClass.initializeApp();await baseClass.loginWithValidCredentials($); patrolTest(" Test 2",($) async {});});

I am also experiencing this issue when using a group(). We're using bdd_widget_test to generate our test files from cucumber format files (feature_name.feature) using Gherkin syntax, but the moment we run single or multiple test file(s) patrol runs smoothly and finishes the test but then it never completes (it get hanged).

@bartekpacia could #1634 resolve this when released?

@bartekpacia
Copy link
Contributor Author

Currently you cannot use group()s in tests, but #1634, once merged, will resolve it.

@bartekpacia
Copy link
Contributor Author

Let's consider this done. Minor follow-ups were split into #1713 and #1714.

@roscadalexandru
Copy link

roscadalexandru commented Oct 20, 2023

I am using the latest versions of patrol and patrol_cli.
I have this file with 2 tests inside a group. I am calling setUpAll to instantiate the bloc I need only once for all tests. I am doing this because what happens in the first test updates the state of the bloc so the second test can also take advantage of the new state. But it seems that the bloc is being reset and it doesn't keep its state. Am I using groups with patrol wrong? I am a bit confused.

Update:

import 'package:flutter_test/flutter_test.dart';
import 'package:patrol/patrol.dart';

void main() {
  late int number;

  setUpAll(() => number = 0);

  group('GroupTest', () {
    patrolTest('FirstPatrol', nativeAutomation: true, ($) async {
      number++;
      expect(number, 1);
    });
    patrolTest('SecondPatrol', ($) async {
      number++;
      expect(number, 2);
    });
    group('InsideGroupTest', () {
      patrolTest('FirstPatrol', ($) async {
        number++;
        expect(number, 3);
      });
      patrolTest('SecondPatrol', ($) async {
        number++;
        expect(number, 4);
      });
    });
  });
}

I found out by running this code that if nativeAutomation is set to true, for the next tests setUpAll will be recalled once again.
The last 3 tests will fail because each time the number will be smaller by one than the expected value.

@roscadalexandru
Copy link

@bartekpacia any thought on my last comment?

Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar problem, please file a new issue. Make sure to follow the template and provide all the information necessary to reproduce the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature New feature request package: patrol Related to the patrol package (native automation, test bundling)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants