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

[feat] Load workspace during integration test from a file. #2106

Closed
wants to merge 24 commits into from

Conversation

a-wallen
Copy link
Contributor

@a-wallen a-wallen commented Mar 25, 2023

Uses a workspace uploaded from a .zip file during an integration test. Before any tests are run, the TestWorkspaceService unzips the workspace folder into .ephemeral/$name. This folder is ignored by source. After the workspace is unzipped, the TestWorkspaceService tells the application to read the workspace from .ephemeral/$name

If contributors need to write tests for Kanban board, they can create a new AppFlowy workspace with just a Kanban board, compress the folder, and add it to test_workspaces under integration_test.

@a-wallen a-wallen force-pushed the load_workspace_integration branch from 8c8a974 to 21c1b25 Compare March 25, 2023 01:24
@a-wallen
Copy link
Contributor Author

fixes #2085

@a-wallen
Copy link
Contributor Author

I'd like to be able to delete the workspace when the integration test are being torn down, but since the application is still open and reading the workspace file, the operating system blocks the operation.

@annieappflowy annieappflowy added tests tasks related to writing tests infra engineering tasks that are not related to UX/UI features labels Mar 28, 2023
@a-wallen a-wallen marked this pull request as ready for review March 29, 2023 01:10
@a-wallen a-wallen changed the title [feat] wip: load workspace during integration test from a file. [feat] Load workspace during integration test from a file. Mar 29, 2023
final File out = File(p.join(parent.path, '$_name.zip'));
if (await out.exists()) return out;
await out.create();
final ByteData data = await rootBundle.load(_asset);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ran into issues reading files directly from the file system. There can be discrepancies between the local machine and the ones that run these tests on GitHub Actions. See this issue.

@@ -110,11 +110,11 @@ jobs:
working-directory: frontend/appflowy_flutter
run: |
if [ "$RUNNER_OS" == "Linux" ]; then
flutter test integration_test -d Linux --coverage
flutter test integration_test/runner.dart -d Linux --coverage
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR adds a new integration test file. Although running flutter test integration test [-args] should work here, it appears that integration tests are unhappy when split between files. We should keep integration tests separated for readability and we can run them all from one test file runner.dart. See this issue for context.

@a-wallen a-wallen requested a review from LucasXu0 March 29, 2023 01:15
@a-wallen a-wallen force-pushed the load_workspace_integration branch from f50dc78 to 431c716 Compare March 29, 2023 01:34
@@ -165,6 +166,7 @@ flutter:
- assets/images/common/
- assets/images/grid/setting/
- assets/translations/
- assets/test/workspaces/
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you put the test resource here, won't it be included in the release product?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's correct. Tomorrow, I'll try to see whether I can exclude those assets from the release build.

Copy link
Contributor Author

@a-wallen a-wallen Mar 29, 2023

Choose a reason for hiding this comment

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

IIUC, files outside the lib directory won't be included in the build unless referenced in the pubspec.yaml. Hence, there needs to be some mechanism that removes this line from the pubspec.yaml for the release build.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could be an idea to have integration tests in a separate package (integration test package).

Otherwise we can write a script to amend the pubspec yaml, in that case the CI script should output it and await approval before releases, which is not that nice..

Copy link
Contributor Author

@a-wallen a-wallen Mar 29, 2023

Choose a reason for hiding this comment

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

I think that having the integration tests as a separate package is a nice idea. It's better than writing a script to modify the pubspec.yaml. That'll be confusing for someone that doesn't have context. I've never had to pull an integration test suite into a separate package, so I'll let you know how that goes @Xazin. Thanks for your reviews.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So, we're going to separate the integration_test package now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried, but the integration tests can't run without a flutter project so it's not a good option anymore.

Along the same lines, we can create a separate repo/branch that excludes the test assets with a .gitignore file. Another option is to use a build script to remove the files and updates the pubspec.yaml before running flutter build.

@a-wallen
Copy link
Contributor Author

Per @appflowy's request, the PR will use a build script to ignore the test assets in release. Assets between the BEGIN: EXCLUDE_IN_RELEASE and END_EXCLUDE_IN_RELEASE tags will be "removed" before building. To build the application, either run the dart script in frontend/scripts/flutter_release_build/build_flowy.dart or run the task AF: flutter build release (host_platform).

@a-wallen a-wallen force-pushed the load_workspace_integration branch from 9ffc364 to 784e24f Compare April 3, 2023 05:26
@a-wallen a-wallen force-pushed the load_workspace_integration branch from 25c7c04 to 19d16d5 Compare April 3, 2023 06:46
a-wallen and others added 2 commits April 3, 2023 08:37
Co-authored-by: Mihir <84044317+squidrye@users.noreply.github.com>
@a-wallen a-wallen force-pushed the load_workspace_integration branch from 716d2ec to 17769a9 Compare April 3, 2023 18:40
@a-wallen
Copy link
Contributor Author

a-wallen commented Apr 4, 2023

AppFlowy/AppFlowy#2083 had the changes in this PR and just landed. Closing this since it would be somewhat redundant.

@a-wallen a-wallen closed this Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra engineering tasks that are not related to UX/UI features tests tasks related to writing tests
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants