Skip to content

Prevent test folder deletion on running flutter create --empty on an existing app project #147160

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

Conversation

victoreronmosele
Copy link
Contributor

@victoreronmosele victoreronmosele commented Apr 22, 2024

This PR modifies the flutter create --empty command to not delete the test/ folder when run on an existing app project.

Before:

flutter create my_app --empty
mkdir my_app/test
if test -d my_app/test; then echo "test exists"; else echo "test does not exist"; fi # test exists
flutter create my_app --empty  
if test -d my_app/test; then echo "test exists"; else echo "test does not exist"; fi # test does not exist

After:

flutter create my_app --empty
mkdir my_app/test
if test -d my_app/test; then echo "test exists"; else echo "test does not exist"; fi # test exists
flutter create my_app --empty  
if test -d my_app/test; then echo "test exists"; else echo "test does not exist"; fi # test exists

Fixes #134928

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Apr 22, 2024
@victoreronmosele victoreronmosele changed the title Prevent removal of test directory when recreating an app template wit… Prevent Test Folder Deletion On Running flutter create --empty On An Existing App Project Apr 22, 2024
@victoreronmosele victoreronmosele changed the title Prevent Test Folder Deletion On Running flutter create --empty On An Existing App Project Prevent test folder deletion on running flutter create --empty on an existing app project Apr 22, 2024
@victoreronmosele victoreronmosele force-pushed the preserve-test-folder-on-flutter-create--empty branch 5 times, most recently from c52e53f to 1e0b5f6 Compare April 30, 2024 13:29
@victoreronmosele victoreronmosele marked this pull request as ready for review April 30, 2024 22:26
Comment on lines 2111 to 2128
testUsingContext('does not remove the test/ directory when recreating an application project with the --empty flag', () async {
await _createProject(
projectDir,
<String>['--no-pub', '--empty'],
<String>[],
);

projectDir.childDirectory('test').childFile('example_test.dart').createSync(recursive: true);

return _createProject(
projectDir,
<String>['--no-pub', '--empty'],
<String>['test/example_test.dart'],
);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this test is missing an assertion

Copy link
Contributor Author

@victoreronmosele victoreronmosele May 2, 2024

Choose a reason for hiding this comment

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

_createProject implicitly asserts that test/example_test.dart exists but I agree with adding an explicit assertion too.

I will include it.

@victoreronmosele victoreronmosele force-pushed the preserve-test-folder-on-flutter-create--empty branch 2 times, most recently from 723291e to 9f080df Compare May 3, 2024 11:15
Copy link
Contributor

@andrewkolos andrewkolos left a comment

Choose a reason for hiding this comment

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

LGTM

@andrewkolos andrewkolos requested a review from gspencergoog May 3, 2024 16:36
@gspencergoog
Copy link
Contributor

When not creating a new project, won't this recreate test/widget_test.dart if it doesn't exist?

Ideally, this should probably just never write a file to test at all, and skip the delete step.

@andrewkolos
Copy link
Contributor

andrewkolos commented May 13, 2024

When not creating a new project, won't this recreate test/widget_test.dart if it doesn't exist?

Indeed, this seems to be the case.

Ideally, this should probably just never write a file to test at all, and skip the delete step.

Agreed.

@andrewkolos andrewkolos self-requested a review May 13, 2024 03:19
Copy link
Contributor

@andrewkolos andrewkolos left a comment

Choose a reason for hiding this comment

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

@victoreronmosele victoreronmosele marked this pull request as draft May 21, 2024 00:22
@victoreronmosele
Copy link
Contributor Author

When not creating a new project, won't this recreate test/widget_test.dart if it doesn't exist?

Ideally, this should probably just never write a file to test at all, and skip the delete step.

Agreed. Will make this change.

@victoreronmosele victoreronmosele force-pushed the preserve-test-folder-on-flutter-create--empty branch from 9f080df to f18abb2 Compare May 21, 2024 05:46
@victoreronmosele victoreronmosele force-pushed the preserve-test-folder-on-flutter-create--empty branch from 439892b to a82eada Compare May 21, 2024 23:21
@victoreronmosele victoreronmosele marked this pull request as ready for review May 21, 2024 23:21
@victoreronmosele
Copy link
Contributor Author

victoreronmosele commented May 21, 2024

I've modified the create command so that when run with --empty, it doesn't write to the test/ directory, for either new or existing projects.

@victoreronmosele victoreronmosele force-pushed the preserve-test-folder-on-flutter-create--empty branch from a82eada to 36bd740 Compare May 21, 2024 23:35
Copy link
Contributor

@gspencergoog gspencergoog left a comment

Choose a reason for hiding this comment

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

32384589-a60f0e74-c078-11e7-9bc1-e5b5287aea9d

Thanks!! This is much cleaner, and the tests are great.

@gspencergoog
Copy link
Contributor

@andrewkolos Can you re-review this now that it's been updated?

Copy link
Contributor

@andrewkolos andrewkolos left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the continued work on this!

@andrewkolos andrewkolos added the autosubmit Merge PR when tree becomes green via auto submit App label May 22, 2024
@auto-submit auto-submit bot merged commit 4c929f0 into flutter:master May 22, 2024
130 checks passed
hello-coder-xu added a commit to hello-coder-xu/flutter that referenced this pull request May 23, 2024
…ps://github.com/hello-coder-xu/flutter into fix/_floatingActionButtonVisibilityValue-update

* 'fix/_floatingActionButtonVisibilityValue-update' of https://github.com/hello-coder-xu/flutter:
  fix: update _floatingActionButtonVisibility only if floatingActionButton is not null
  [wiki migration] Remaining pages under docs/about/ (flutter#148782)
  Roll Flutter Engine from b6971cdf14f8 to 8b094fbb94d8 (3 revisions) (flutter#148883)
  Fix the second TextFormField to trigger onTapOutside (flutter#148206)
  Try removing robolectric from `integration_test` tests (flutter#148803)
  Prevent test folder deletion on running `flutter create --empty` on an existing app project (flutter#147160)
  [wiki migration] Tool team pages (flutter#148779)
  Roll Flutter Engine from c89defa55801 to b6971cdf14f8 (6 revisions) (flutter#148819)
  [native_assets] Add support for link hooks (flutter#148474)
  Roll Packages from ba19b24 to 6525441 (12 revisions) (flutter#148864)
  Update tokens to 4.0.0 (flutter#148789)
  Move Linux web_long_running_tests_2_5 to bringup (flutter#148854)
  `CupertinoDialogRoute` leak fix (flutter#148774)
  Marks Windows plugin_test to be flaky (flutter#148835)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 23, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 23, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 23, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request May 23, 2024
flutter/flutter@73bf206...8d955cd

2024-05-23 [email protected] Update `FocusManager` platform check to include iOS (flutter/flutter#148612)
2024-05-23 [email protected] [iOS] fix hot restart with native assets (flutter/flutter#148752)
2024-05-23 [email protected] Roll Flutter Engine from b8b82454e302 to 964f087f288c (8 revisions) (flutter/flutter#148943)
2024-05-23 [email protected] Fix DecoratedSliver sample code to reflect the description (flutter/flutter#148621)
2024-05-23 [email protected] Test raw autocomplete api examples (flutter/flutter#148234)
2024-05-23 [email protected] Add test for scaffold.0.dart and scaffold.2.dart (flutter/flutter#148166)
2024-05-23 [email protected] Add tests for restorable_value.0.dart API example. (flutter/flutter#148676)
2024-05-23 [email protected] Roll Flutter Engine from 8b094fbb94d8 to b8b82454e302 (6 revisions) (flutter/flutter#148919)
2024-05-22 [email protected] Allow `RenderObject.getTransformTo` to take an arbitrary RenderObject in the same tree (flutter/flutter#148897)
2024-05-22 [email protected] 3.22.1 changelog updates (flutter/flutter#148895)
2024-05-22 [email protected] Add frame number and widget location map service extension (flutter/flutter#148702)
2024-05-22 [email protected] Remove an assert with false positives (flutter/flutter#148795)
2024-05-22 [email protected] Revert "Fix the second TextFormField to trigger onTapOutside" (flutter/flutter#148909)
2024-05-22 [email protected] [wiki migration] Remaining pages under docs/about/ (flutter/flutter#148782)
2024-05-22 [email protected] Roll Flutter Engine from b6971cdf14f8 to 8b094fbb94d8 (3 revisions) (flutter/flutter#148883)
2024-05-22 [email protected] Fix the second TextFormField to trigger onTapOutside (flutter/flutter#148206)
2024-05-22 [email protected] Try removing robolectric from `integration_test` tests (flutter/flutter#148803)
2024-05-22 [email protected] Prevent test folder deletion on running `flutter create --empty` on an existing app project (flutter/flutter#147160)
2024-05-22 [email protected] [wiki migration] Tool team pages (flutter/flutter#148779)
2024-05-22 [email protected] Roll Flutter Engine from c89defa55801 to b6971cdf14f8 (6 revisions) (flutter/flutter#148819)
2024-05-22 [email protected] [native_assets] Add support for link hooks (flutter/flutter#148474)
2024-05-22 [email protected] Roll Packages from ba19b24 to 6525441 (12 revisions) (flutter/flutter#148864)
2024-05-22 [email protected] Update tokens to 4.0.0 (flutter/flutter#148789)
2024-05-22 [email protected] Move Linux web_long_running_tests_2_5 to bringup (flutter/flutter#148854)

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
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

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

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
victorsanni pushed a commit to victorsanni/flutter that referenced this pull request May 31, 2024
…n existing app project (flutter#147160)

This PR modifies the `flutter create --empty` command to not delete the `test/` folder when run on an existing app project. 

Before:
```bash
flutter create my_app --empty
mkdir my_app/test
if test -d my_app/test; then echo "test exists"; else echo "test does not exist"; fi # test exists
flutter create my_app --empty  
if test -d my_app/test; then echo "test exists"; else echo "test does not exist"; fi # test does not exist
```

After:
```bash
flutter create my_app --empty
mkdir my_app/test
if test -d my_app/test; then echo "test exists"; else echo "test does not exist"; fi # test exists
flutter create my_app --empty  
if test -d my_app/test; then echo "test exists"; else echo "test does not exist"; fi # test exists
```
Fixes flutter#134928
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using flutter create to repair a project with the --empty option deletes test folder
3 participants