feat(test): enable running very_good test --optimization for specific files/directory by agacemi · Pull Request #564 · VeryGoodOpenSource/very_good_cli

agacemi · 2022-11-12T07:45:14Z

Description

In the current version, optimization is disabled if we give additional directoires or files to very_good test command line. The goal of this PR is to add the ability to run `'very_good test' with optimization on a specific files or directoriesc or both.

# Optimization will be enabled
very_good test  test/counter/cubit
very_good test  test/counter/view/counter_view_test.dart  
very_good test  test/counter/view  test/counter/cubit

Type of Change

✨ New feature (non-breaking change which adds functionality)
🛠️ Bug fix (non-breaking change which fixes an issue)
❌ Breaking change (fix or feature that would cause existing functionality to change)
🧹 Code refactor
✅ Build configuration change
📝 Documentation
🗑️ Chore

… files/directories

renancaraujo

I left here some remarks about the overall implementation.

Thanks for the time you took for this. It may be better to have an issue before a PR so we can discuss and understand better on top of the problem.

Also, your implementation seems to be breaking the e2e test (which proves that this is a breaking change), may be fas to fix (just remove the path argument).

If you want to follow up on this, wait for a couple of days when the test_runner brick will be unbundled in this repo (so things may be easier to review and update).

renancaraujo · 2023-01-19T19:56:56Z

lib/src/commands/test/templates/test_runner_bundle.dart

      "path": "pre_gen.dart",
      "data":
-          "aW1wb3J0ICdkYXJ0OmlvJzsKCmltcG9ydCAncGFja2FnZTptYXNvbi9tYXNvbi5kYXJ0JzsKaW1wb3J0ICdwYWNrYWdlOnBhdGgvcGF0aC5kYXJ0JyBhcyBwYXRoOwoKRnV0dXJlPHZvaWQ+IHJ1bihIb29rQ29udGV4dCBjb250ZXh0KSBhc3luYyB7CiAgZmluYWwgcGFja2FnZVJvb3QgPSBjb250ZXh0LnZhcnNbJ3BhY2thZ2Utcm9vdCddOwogIGZpbmFsIHRlc3REaXIgPSBEaXJlY3RvcnkocGF0aC5qb2luKHBhY2thZ2VSb290LCAndGVzdCcpKTsKCiAgaWYgKCF0ZXN0RGlyLmV4aXN0c1N5bmMoKSkgewogICAgY29udGV4dC5sb2dnZXIuZXJyKCdDb3VsZCBub3QgZmluZCBkaXJlY3RvcnkgJHt0ZXN0RGlyLnBhdGh9Jyk7CiAgICBleGl0KDEpOwogIH0KCiAgZmluYWwgcHVic3BlYyA9IEZpbGUocGF0aC5qb2luKHBhY2thZ2VSb290LCAncHVic3BlYy55YW1sJykpOwogIGlmICghcHVic3BlYy5leGlzdHNTeW5jKCkpIHsKICAgIGNvbnRleHQubG9nZ2VyLmVycignQ291bGQgbm90IGZpbmQgcHVic3BlYy55YW1sIGF0ICR7dGVzdERpci5wYXRofScpOwogICAgZXhpdCgxKTsKICB9CgogIGZpbmFsIHB1YnNwZWNDb250ZW50cyA9IGF3YWl0IHB1YnNwZWMucmVhZEFzU3RyaW5nKCk7CiAgZmluYWwgZmx1dHRlclNka1JlZ0V4cCA9IFJlZ0V4cChyJ3NkazpccypmbHV0dGVyJCcsIG11bHRpTGluZTogdHJ1ZSk7CiAgZmluYWwgaXNGbHV0dGVyID0gZmx1dHRlclNka1JlZ0V4cC5oYXNNYXRjaChwdWJzcGVjQ29udGVudHMpOwoKICBmaW5hbCB0ZXN0cyA9IHRlc3REaXIKICAgICAgLmxpc3RTeW5jKHJlY3Vyc2l2ZTogdHJ1ZSkKICAgICAgLndoZXJlKChlbnRpdHkpID0+IGVudGl0eS5pc1Rlc3QpCiAgICAgIC5tYXAoKGVudGl0eSkgPT4KICAgICAgICAgIHBhdGgucmVsYXRpdmUoZW50aXR5LnBhdGgsIGZyb206IHRlc3REaXIucGF0aCkucmVwbGFjZUFsbChyJ1wnLCAnLycpKQogICAgICAudG9MaXN0KCk7CgogIGNvbnRleHQudmFycyA9IHsndGVzdHMnOiB0ZXN0cywgJ2lzRmx1dHRlcic6IGlzRmx1dHRlcn07Cn0KCmV4dGVuc2lvbiBvbiBGaWxlU3lzdGVtRW50aXR5IHsKICBib29sIGdldCBpc1Rlc3QgewogICAgcmV0dXJuIHRoaXMgaXMgRmlsZSAmJiBwYXRoLmJhc2VuYW1lKHRoaXMucGF0aCkuZW5kc1dpdGgoJ190ZXN0LmRhcnQnKTsKICB9Cn0K",
+          "aW1wb3J0ICdkYXJ0OmlvJzsKCmltcG9ydCAncGFja2FnZTptYXNvbi9tYXNvbi5kYXJ0JzsKaW1wb3J0ICdwYWNrYWdlOnBhdGgvcGF0aC5kYXJ0JyBhcyBwYXRoOwoKRnV0dXJlPHZvaWQ+IHJ1bihIb29rQ29udGV4dCBjb250ZXh0KSBhc3luYyB7CiAgZmluYWwgcGFja2FnZVJvb3QgPSBjb250ZXh0LnZhcnNbJ3BhY2thZ2Utcm9vdCddOwogIGZpbmFsIHRlc3RQYXRocyA9IGNvbnRleHQudmFyc1sndGVzdC1wYXRocyddOwogIGZpbmFsIHRlc3REaXIgPSBEaXJlY3RvcnkocGF0aC5qb2luKHBhY2thZ2VSb290LCAndGVzdCcpKTsKCiAgaWYgKCF0ZXN0RGlyLmV4aXN0c1N5bmMoKSkgewogICAgY29udGV4dC5sb2dnZXIuZXJyKCdDb3VsZCBub3QgZmluZCBkaXJlY3RvcnkgJHt0ZXN0RGlyLnBhdGh9Jyk7CiAgICBleGl0KDEpOwogIH0KCiAgZmluYWwgcHVic3BlYyA9IEZpbGUocGF0aC5qb2luKHBhY2thZ2VSb290LCAncHVic3BlYy55YW1sJykpOwogIGlmICghcHVic3BlYy5leGlzdHNTeW5jKCkpIHsKICAgIGNvbnRleHQubG9nZ2VyLmVycignQ291bGQgbm90IGZpbmQgcHVic3BlYy55YW1sIGF0ICR7dGVzdERpci5wYXRofScpOwogICAgZXhpdCgxKTsKICB9CgogIGZpbmFsIHB1YnNwZWNDb250ZW50cyA9IGF3YWl0IHB1YnNwZWMucmVhZEFzU3RyaW5nKCk7CiAgZmluYWwgZmx1dHRlclNka1JlZ0V4cCA9IFJlZ0V4cChyJ3NkazpccypmbHV0dGVyJCcsIG11bHRpTGluZTogdHJ1ZSk7CiAgZmluYWwgaXNGbHV0dGVyID0gZmx1dHRlclNka1JlZ0V4cC5oYXNNYXRjaChwdWJzcGVjQ29udGVudHMpOwoKICBmaW5hbCB0ZXN0cyA9IHRlc3REaXIKICAgICAgLmxpc3RTeW5jKHJlY3Vyc2l2ZTogdHJ1ZSkKICAgICAgLndoZXJlKChlbnRpdHkpID0+IGVudGl0eS5pc1Rlc3QpCiAgICAgIC53aGVyZSgoZW50aXR5KSA9PgogICAgICAgICAgdGVzdFBhdGhzLmlzRW1wdHkgfHwKICAgICAgICAgIHRlc3RQYXRocy5hbnkoKHRlc3RQYXRoKSA9PgogICAgICAgICAgICAgIHBhdGgucmVsYXRpdmUoZW50aXR5LnBhdGgpLnN0YXJ0c1dpdGgodGVzdFBhdGgpKSkKICAgICAgLm1hcCgoZW50aXR5KSA9PgogICAgICAgIHBhdGgucmVsYXRpdmUoZW50aXR5LnBhdGgsIGZyb206IHRlc3REaXIucGF0aCkucmVwbGFjZUFsbChyJ1wnLCAnLycpKQogICAgICAudG9MaXN0KCk7CgogIGNvbnRleHQudmFycyA9IHsndGVzdHMnOiB0ZXN0cywgJ2lzRmx1dHRlcic6IGlzRmx1dHRlcn07Cn0KCmV4dGVuc2lvbiBvbiBGaWxlU3lzdGVtRW50aXR5IHsKICBib29sIGdldCBpc1Rlc3QgewogICAgcmV0dXJuIHRoaXMgaXMgRmlsZSAmJiBwYXRoLmJhc2VuYW1lKHRoaXMucGF0aCkuZW5kc1dpdGgoJ190ZXN0LmRhcnQnKTsKICB9Cn0=",


Seems like ithis pr adds a parameter to the rest runner that is not listed on the brick.yaml file

renancaraujo · 2023-01-19T19:59:13Z

lib/src/commands/test/test.dart

            '--no-pub',
-            ..._argResults.rest,
+            if (!isOptimizationApplied) ..._argResults.rest,


This is a breaking change. People that passed trailing arguments (previously ignored) now may face some errors.

Which is the case for our e2e tests

renancaraujo · 2023-01-19T20:00:30Z

test/src/cli/flutter_cli_test.dart

        final directory = Directory.systemTemp.createTempSync();
-        final originalVars = <String, dynamic>{'package-root': directory.path};
+        final originalVars = <String, dynamic>{
+          'package-root': directory.path,
+          'test-paths': <String>[],
+        };
        final updatedVars = <String, dynamic>{
          'package-root': directory.path,
-          'foo': 'bar'
+          'foo': 'bar',
+          'test-paths': <String>[]
        };


Need a new test with some paths

agacemi · 2023-01-23T22:38:53Z

I left here some remarks about the overall implementation.

Thanks for the time you took for this. It may be better to have an issue before a PR so we can discuss and understand better on top of the problem.

Also, your implementation seems to be breaking the e2e test (which proves that this is a breaking change), may be fas to fix (just remove the path argument).

If you want to follow up on this, wait for a couple of days when the test_runner brick will be unbundled in this repo (so things may be easier to review and update).

Thank you for reviewing.

At first, I wanted to solve the issue #508. On other hand, our team expressed this need : We have more than 4000 tests. Our base code is organized around many folders, each folder deals with a feature managed by a team. Thus, we wanted to speed up tests by enabling optimization if very_good was run on a given folder. Hence, my PR.

Regarding the breaking change, I have not idea what could be trailing arguments except directories or files path ?! This PR assume that the rest of result, if given, are necessarily specific files or specific directories. Maybe I was wrong

Please, let me know when test_runner brick is unbundled to resume some fix you requested.

renancaraujo · 2023-02-13T13:34:43Z

Please, let me know when test_runner brick is unbundled to resume some fix you requested.

#639 is merged so that the changes can be done to the test_opitimizer brick and bundled via tool/generate_test_optimizer_bundle.sh

renancaraujo · 2023-02-27T19:44:21Z

@agacemi do you plan to followup on this?

agacemi · 2023-03-01T14:59:15Z

@renancaraujo Yes absolutely. I'am just waiting for the acceptation of merge on my other PR #563 to avoid rebase hell :) and approval dismissing.

tomarra · 2023-10-26T20:06:43Z

@agacemi looking for an update on this PR from your side. It's been sitting for some time so it has some merge conflicts that need to be solved.

agacemi · 2023-10-26T21:02:18Z

@agacemi looking for an update on this PR from your side. It's been sitting for some time so it has some merge conflicts that need to be solved.

@tomarra Absolutely, I'm aware. Currently, I can't spent time to maintain to follow up this PR. for this reason, it's in draft status

tomarra · 2023-10-27T13:49:22Z

@tomarra Absolutely, I'm aware. Currently, I can't spent time to maintain to follow up this PR. for this reason, it's in draft status

Thanks for the update @agacemi. Given that I'm going to close this for now so it doesn't continue to popup in our dashboards. When you do have time to work on this feel free to either reopen this with the needed changes or start a new PR.

agacemi requested review from felangel, renancaraujo and wolfenrain as code owners November 12, 2022 07:45

agacemi force-pushed the test-from-specific-file branch from 641b42c to 53835f2 Compare December 17, 2022 07:39

feat(test): enable running very_good test --optimization for specific…

6aa418e

… files/directories

agacemi force-pushed the test-from-specific-file branch from 53835f2 to 6aa418e Compare December 17, 2022 23:33

renancaraujo self-assigned this Jan 3, 2023

Merge branch 'main' into test-from-specific-file

206d4f8

renancaraujo removed their assignment Jan 19, 2023

renancaraujo reviewed Jan 19, 2023

View reviewed changes

renancaraujo added the waiting for response Waiting for follow up label Jan 19, 2023

agacemi marked this pull request as draft March 9, 2023 18:40

renancaraujo force-pushed the main branch from c7c7f56 to fb998d0 Compare May 23, 2023 19:21

tomarra closed this Oct 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(test): enable running very_good test --optimization for specific files/directory#564

feat(test): enable running very_good test --optimization for specific files/directory#564
agacemi wants to merge 2 commits intoVeryGoodOpenSource:mainfrom
agacemi:test-from-specific-file

agacemi commented Nov 12, 2022

Uh oh!

renancaraujo left a comment

Uh oh!

renancaraujo Jan 19, 2023

Uh oh!

renancaraujo Jan 19, 2023

Uh oh!

renancaraujo Jan 19, 2023

Uh oh!

agacemi commented Jan 23, 2023 •

edited

Loading

Uh oh!

renancaraujo commented Feb 13, 2023

Uh oh!

renancaraujo commented Feb 27, 2023

Uh oh!

agacemi commented Mar 1, 2023

Uh oh!

tomarra commented Oct 26, 2023

Uh oh!

agacemi commented Oct 26, 2023 •

edited

Loading

Uh oh!

tomarra commented Oct 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

agacemi commented Nov 12, 2022

Description

Type of Change

Uh oh!

renancaraujo left a comment

Choose a reason for hiding this comment

Uh oh!

renancaraujo Jan 19, 2023

Choose a reason for hiding this comment

Uh oh!

renancaraujo Jan 19, 2023

Choose a reason for hiding this comment

Uh oh!

renancaraujo Jan 19, 2023

Choose a reason for hiding this comment

Uh oh!

agacemi commented Jan 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

renancaraujo commented Feb 13, 2023

Uh oh!

renancaraujo commented Feb 27, 2023

Uh oh!

agacemi commented Mar 1, 2023

Uh oh!

tomarra commented Oct 26, 2023

Uh oh!

agacemi commented Oct 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomarra commented Oct 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

agacemi commented Jan 23, 2023 •

edited

Loading

agacemi commented Oct 26, 2023 •

edited

Loading