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.

Sign up for GitHub

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

Jump to bottom

Add script to test CDN bundle #216

Merged
merged 3 commits into from
Oct 19, 2023
Merged

Add script to test CDN bundle #216

merged 3 commits into from
Oct 19, 2023

Conversation

lawrence-forooghian
Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian commented Oct 10, 2023 •
edited
Loading

Add a test:cdn_bundle NPM script and run it in CI. This script tests the bundle that the .github/workflows/cdn.yml workflow will upload to the CDN, by checking that, when this bundle is imported by a web page, it gives access to a working Spaces client.

The approach (using Playwright and a local web server powered by Express) is based heavily on that used by the test:playwright NPM script in ably-js. I think we’ll also be able to reuse some of this when we come to implement MMB-218 (integration tests).

@lawrence-forooghian lawrence-forooghian changed the base branch from main to remove-LockAttributes October 10, 2023 13:39
@lawrence-forooghian lawrence-forooghian force-pushed the MMB-326-test-cdn-build branch 3 times, most recently from 67abbad to 8b7bfe2 Compare October 10, 2023 17:26
@lawrence-forooghian lawrence-forooghian mentioned this pull request Oct 10, 2023
Draft
@lawrence-forooghian lawrence-forooghian changed the title Mmb 326 test cdn build Add script to test CDN bundel Oct 10, 2023
@lawrence-forooghian lawrence-forooghian changed the title Add script to test CDN bundel Add script to test CDN bundle Oct 10, 2023
@lawrence-forooghian lawrence-forooghian marked this pull request as ready for review October 10, 2023 18:05
@lawrence-forooghian lawrence-forooghian force-pushed the MMB-326-test-cdn-build branch 4 times, most recently from 1b41236 to 3a256d6 Compare October 10, 2023 20:19
dpiatek
dpiatek suggested changes Oct 13, 2023
View reviewed changes
Copy link
Contributor

@dpiatek dpiatek left a comment

Choose a reason for hiding this comment

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

This is a great start @lawrence-forooghian , however I think we could be using more of playwright features. I feel the way the test is setup, we are using it more as browser driver instead of test runner. I tried to do some refactoring in #218. Have a look and let's chat about this on Monday.

README.md
```ts
const client = new Ably.Realtime.Promise({ key: "<API-key>", clientId: "<client-ID>" });
const spaces = new Spaces(client);
```
Copy link
Contributor

Choose a reason for hiding this comment

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

we should add this also to the docs/usage.md document as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, done.

dpiatek
dpiatek reviewed Oct 13, 2023
View reviewed changes
.gitmodules
@@ -0,0 +1,3 @@
[submodule "test/ably-common"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need some instructions in place so folks know to run git submodules (and install chromium) before running this test locally

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, have added a section to CONTRIBUTING.md.

Base automatically changed from remove-LockAttributes to main October 16, 2023 11:47
@lawrence-forooghian
Copy link
Collaborator Author

This is a great start @lawrence-forooghian , however I think we could be using more of playwright features. I feel the way the test is setup, we are using it more as browser driver instead of test runner. I tried to do some refactoring in #218. Have a look and let's chat about this on Monday.

Oh, that's cool — I didn't know that functionality existed in Playwright. I'll take a look at your linked PR and try and update this PR in line with it.

@lawrence-forooghian lawrence-forooghian force-pushed the MMB-326-test-cdn-build branch 4 times, most recently from b6ccef2 to be251c8 Compare October 16, 2023 14:56
@lawrence-forooghian
Copy link
Collaborator Author

@dpiatek I've incorporated your suggested changes from #218.

@lawrence-forooghian
Copy link
Collaborator Author

@dpiatek as mentioned above, MMB-339 is currently causing CI to intermittently fail on this branch, meaning we can't merge it yet. How would you like to proceed with this? I was previously told not to investigate that issue until you'd had a chance to read it.

@dpiatek
Copy link
Contributor

dpiatek commented Oct 16, 2023

@dpiatek as mentioned above, MMB-339 is currently causing CI to intermittently fail on this branch, meaning we can't merge it yet. How would you like to proceed with this? I was previously told not to investigate that issue until you'd had a chance to read it.

Let's bring it into the sprint and try to investigate. I wonder if it could be a sandbox issue?

@dpiatek dpiatek mentioned this pull request Oct 16, 2023
Closed
@lawrence-forooghian
Copy link
Collaborator Author

Let's bring it into the sprint and try to investigate.

Cool, have brought it into the sprint.

@github-actions github-actions bot temporarily deployed to staging/pull/216/typedoc October 17, 2023 12:48 Inactive
@lawrence-forooghian lawrence-forooghian changed the base branch from main to COL-335-space.enter-hanging October 18, 2023 17:48
@github-actions github-actions bot temporarily deployed to staging/pull/216/typedoc October 18, 2023 17:48 Inactive
@lawrence-forooghian
Copy link
Collaborator Author

@dpiatek this PR is now based on top of #227 and so the issue mentioned here no longer exists.

@lawrence-forooghian lawrence-forooghian force-pushed the COL-335-space.enter-hanging branch 5 times, most recently from c8d9e25 to 803416d Compare October 19, 2023 16:07
Base automatically changed from COL-335-space.enter-hanging to main October 19, 2023 16:09
@lawrence-forooghian
Fix CDN instructions in README and usage docs
ac9c6c7 
When used via the CDN, ably-js exposes itself through a global `Ably`
object.
@lawrence-forooghian
Update Prettier to 3.0.3
4d31d03 
I am going to introduce a submodule and want to make use of a
.prettierignore file to tell Prettier to ignore it. However, we are
already overriding the --ignore-path flag to tell it to ignore the files
described by .gitignore.

Prettier 3 ignores the files described by .gitignore by default. This
means that we can now remove our usage of --ignore-path and it’ll ignore
the files described by .gitignore _and_ .prettierignore.
@lawrence-forooghian
Add a script to test the bundle that will be uploaded to CDN
e3a2f4d 
Add a `test:cdn_bundle` NPM script and run it in CI. This script tests
the bundle that the `.github/workflows/cdn.yml` workflow will upload to
the CDN, by checking that, when this bundle is imported by a web page,
it gives access to a working Spaces client.

The approach (using Playwright and a local web server powered by
Express) is based heavily on that used by the `test:playwright` NPM
script in ably-js at commit 83b4516. I then incorporated Dom’s
suggestion of using Playwright’s built-in test-running functionality,
taking code from his draft PR #218.
@lawrence-forooghian lawrence-forooghian merged commit fa95f9f into main Oct 19, 2023
7 checks passed
@lawrence-forooghian lawrence-forooghian deleted the MMB-326-test-cdn-build branch October 19, 2023 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpiatek dpiatek dpiatek approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lawrence-forooghian @dpiatek