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

Draft Playwright test cases! #10

Merged
merged 31 commits into from
Apr 6, 2022
Merged

Draft Playwright test cases! #10

merged 31 commits into from
Apr 6, 2022

Conversation

ajhyndman
Copy link
Member

@ajhyndman ajhyndman force-pushed the initial-test-cases branch 2 times, most recently from 026d7a9 to cd05f32 Compare March 29, 2022 18:49
@@ -0,0 +1,2 @@
/** Assert that a metric is triggered within +/- 200ms */
export const FUDGE = 200;
Copy link
Contributor

Choose a reason for hiding this comment

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

Love the name of this constant 😁

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, haha. I'm not sure what the best way to assert expected TTVC values is. There's always going to be some intrinsic variability between test runs.

If this pattern makes sense, I think we could define a custom test matcher that asserts that something happens within a standard "fudge factor" of the expected time.

Another idea I had was maybe the .html source file should signal to the test process the point at which VC is expected to occur? I haven't worked out the logistics of that yet.

test/e2e/images2.spec.ts Outdated Show resolved Hide resolved
@ajhyndman ajhyndman changed the base branch from jest to main March 31, 2022 02:05
app.use('/dist', express.static(path.join(__dirname, '../../dist')));
// allow loading libraries from node_modules
app.use('/node_modules', express.static(path.join(__dirname, '../../node_modules')));
Copy link
Member Author

Choose a reason for hiding this comment

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

This is maybe a weird choice.

Maybe it's a mistake, but I thought I would try to draft a few actual React application test cases. I still don't want any tests to have to rely on a CI environment having access to the internet, so I added /node_modules to the set of express static paths.

This lets you use a script tag something like the following in a test case:

<script src="/node_modules/react/umd/react.production.min.js"></script>

@ajhyndman
Copy link
Member Author

@zubair-io I reorganized the playwright test cases into folders now! I think you're right, it will scale better.

image

@ajhyndman ajhyndman changed the title Define a dozen new test cases! Draft Playwright test cases! Apr 1, 2022
@ajhyndman
Copy link
Member Author

I pushed another dozen test cases today!

Comment on lines 111 to 113
if (loadingImages.size === 0) {
return Promise.resolve(performance.now());
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I am pushing this fix in production as well. D:

https://phabricator.dropboxer.net/D848659

It's so hard to spot these bugs just by running the code directly against Dropbox.

@Jonathan1600
Copy link
Collaborator

I was running the tests and they seem to still be flaky while multi threaded. We should probably adress a good configuration for running the tests without being that flaky.

@Jonathan1600
Copy link
Collaborator

I was testing and it seemed to work way more with only 3 workers without being painfully slow, maybe we could consider as setting that as the default?

@ajhyndman
Copy link
Member Author

@Jonathan1600 Let's try that!

@ajhyndman ajhyndman merged commit 8cc4901 into main Apr 6, 2022
@ajhyndman ajhyndman deleted the initial-test-cases branch April 6, 2022 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants