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

fix: end test waiter when using @animationData arg #441

Merged
merged 1 commit into from
Apr 16, 2024
Merged

Conversation

22a
Copy link
Contributor

@22a 22a commented Mar 14, 2024

This PR ends the test waiter (inside the <Lottie> component's animate action) after reading the @animationData arg.

Prior to this change, the test waiter is started when using @path or @animationData:

const token = waiter.beginAsync();

but it was only ended when using @path - so specs relying on @animationData hang and timeout:

waiter.endAsync(token);

Testing

I have patch-package'd this change into our app and can confirm it works as expected:

Screenshot of passing spec after patch failed import:

Screenshot of passing spec after applying the patch in this PR

I attempted to write a failing spec to reproduce the @animationData test waiter bug but couldn't find an ergonomic way to provide a reasonable @animationData payload without checking in a big bag of JSON. In this branch I tried to reach up into the public/ directory where you already have a suitable json payload but I ran into import issues (see details below) and it already involved fiddling with the test app's tsconfig.json so I parked it. If you're okay with me tweaking tsconfig and duplicating test-app/public/data.json somewhere under app/ I'm happy to prep a PR to add a suitable spec.

Details on test-app/public/data.json failed import

[test:ember] not ok 1 Chrome 122.0 - [1 ms] - TestLoader Failures: test-app/tests/integration/components/lottie-test: could not be loaded
│ [test:ember]     ---
│ [test:ember]         actual: >
│ [test:ember]             null
│ [test:ember]         stack: >
│ [test:ember]             Error: Could not find module `test-app/public/data.json` imported from `test-app/tests/integration/components/lottie-test`
│ [test:ember]                 at missingModule (http://localhost:7357/assets/vendor.js:266:11)
│ [test:ember]                 at findModule (http://localhost:7357/assets/vendor.js:277:7)
│ [test:ember]                 at Module.findDeps (http://localhost:7357/assets/vendor.js:187:24)
│ [test:ember]                 at findModule (http://localhost:7357/assets/vendor.js:281:11)
│ [test:ember]                 at requireModule (http://localhost:7357/assets/vendor.js:43:15)
│ [test:ember]                 at TestLoader.require (http://localhost:7357/assets/test-support.js:6981:9)
│ [test:ember]                 at TestLoader.loadModules (http://localhost:7357/assets/test-support.js:6975:14)
│ [test:ember]                 at loadTests (http://localhost:7357/assets/chunk.vendors-node_modules_pnpm_ember-qunit_8_0_2__ember_test-helpers_3_3_0__glint_template_1_3_0_e-8ce205.7e0dc8cc4603a627299a.js:320:42)
│ [test:ember]                 at start (http://localhost:7357/assets/chunk.vendors-node_modules_pnpm_ember-qunit_8_0_2__ember_test-helpers_3_3_0__glint_template_1_3_0_e-8ce205.7e0dc8cc4603a627299a.js:214:119)
│ [test:ember]                 at Module.callback (http://localhost:7357/assets/tests.js:395:25)

@anas7asia anas7asia added bug Something isn't working good first issue Good for newcomers labels Mar 20, 2024
@22a
Copy link
Contributor Author

22a commented Apr 2, 2024

Hey @anas7asia, is there anything you'd like me to change about this PR?

@oreqizer
Copy link
Contributor

added a test here 22a/ember-lottie@main...oreqizer:ember-lottie:main

I reverted the change and the test indeed failed. adding this commit's change fixes the test 👍

@anas7asia
Copy link
Contributor

Hello @22a!
Thank you for the changes you've proposed. Let's merge 22a#1 (review) proposed by @oreqizer first to your fork and then merge this PR 🙌

@22a
Copy link
Contributor Author

22a commented Apr 12, 2024

Excellent, thank you @oreqizer @anas7asia - 22a#1 has been merged

@anas7asia
Copy link
Contributor

Thank you @22a, can you squash your commits before we merge, please?

@22a
Copy link
Contributor Author

22a commented Apr 12, 2024

@anas7asia sure thing, done 👍

@oreqizer
Copy link
Contributor

Hey @22a, the lint job was timing out because of the new fixtures/data.ts file. Apparently, /* eslint-disable */ is not enough there.

This PR should address that by ignoring fixtures in .eslintignore. 🙏

CC @anas7asia

@22a
Copy link
Contributor Author

22a commented Apr 15, 2024

nice one @oreqizer - merged and squashed again

@22a
Copy link
Contributor Author

22a commented Apr 15, 2024

apologies, my previous commit violated your commit message linting. fixed.

@oreqizer
Copy link
Contributor

failing job is unrelated, merging

@oreqizer oreqizer merged commit eaa760f into qonto:main Apr 16, 2024
16 of 17 checks passed
@anas7asia
Copy link
Contributor

@22a, the fix is available in the latest release: https://github.com/qonto/ember-lottie/releases/tag/v1.1.3
Thank you again for submitting your PR! ❤️

cc @oreqizer 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants