Add playwright tests and add to CI #87

ddimaria · 2023-01-30T23:48:23Z

Addresses issue #86

Add Playwright test for the browser-to-server example
Integrate Playwright tests in CI

MarcoPolo

One small thing. Otherwise looks good

examples/browser-to-server/index.js

examples/browser-to-server/tests/test.js

… elements

examples/browser-to-server/package.json

GlenDC · 2023-01-31T20:02:17Z

examples/browser-to-server/tests/test.js

+async function spawnGoLibp2p() {
+  if (!existsSync('../../examples/go-libp2p-server/go-libp2p-server')) {
+    await new Promise((resolve, reject) => {
+      exec('go build',


same here, might as well just always run it, no? instead of building it first?

Good question! this should be documented. go run ./main.go builds the program then starts a subprocess for the program. SIGINT on the parent process doesn't stop the subprocess (try running go run in the background and issuing a kill -2 pid). By running the binary directly we avoid the indirection and SIGINT works as expected.

I wonder if we could SIGKILL this instead and it would work?

I don't think it's worth changing in this PR.

I agree that it is probably out of scope. More of a note.

examples/browser-to-server/package.json

GlenDC · 2023-01-31T20:06:52Z

examples/browser-to-server/package.json

@@ -5,14 +5,20 @@
  "type": "module",


I think it falls out of scope for this PR, but something to think about is that we might as well turn this repo into a workspace. This way the example and webrtc package could be both living in the same package, allowing the example to make use of the @libp2p/webrtc package the same as any external usage would. Making the example also directly copyable without having to rely on relative paths in its package.json to make it work given that the relative paths would be fixed by the root workspace definition :) Just my 2 cents on this matters, but feels cleaner to me.

The pathing has been removed in favor of the npm package import. The example was written before the package was released, so this was something that needed to be done now that it's published. Thanks for the reminder!

Regarding the placement of this comment, is there a concern with "type": "module"?

No no, just decided to put it on top of the file it was related to. Sorry for the confusion there.

GlenDC · 2023-01-31T20:08:32Z

examples/browser-to-server/tests/test.js

+
+play.describe('bundle ipfs with parceljs:', () => {
+  // DOM
+  const connectBtn = '#connect'


2 comments:

is there no convention for constants, in many language it's something like UPPER_CASE but dunno about ts or PL in general. Either way I would expect it to be different from regular variable names?

might want to use a convention suffix for these ones to make clear that these are Html Element Identifier strings rather than actual "buttons", "addresses", "input fields", etc...?

convention suffix for these ones to make clear that these are Html Element Identifier strings rather than actual "buttons"

That's what the types are for :)

What types? O.O

You mean that this could be implied because it is of type str? Dunno, not entirely sure that is the case. Certainly not for the output one imho. I like to be very clear about this thing, but neither do I want to make a foothold here, so I'm fine with whatever you end up choosing here, just note from my POV.

Oh my mistake, I thought this was a .ts file

This was a borrow from the WebTransport Playwright test, though Hungarian(ish) notation may be beneficial here. I'm OK leaving as is.

The notation botters me only minor, so not too worried about that. But when I read the usage for a variable "connectBtn" I do not really link it to something being a (string) ID to an HTML element. That's my main concern here. But not a strong opinion given the minor size of the file, so feel free to take it as you wish.

GlenDC · 2023-01-31T20:09:22Z

examples/browser-to-server/tests/test.js

+  play.beforeAll(async ({ }, testInfo) => {
+    testInfo.setTimeout(5 * 60_000)
+    const s = await spawnGoLibp2p()
+    server = s.server


dunno what the convention is but unpacking { server, serverAddress} = s should work here, no?

No. This is assigning to closured variables.

Yes, these are singletons where the variables are scoped in the play.describe closure. The styleguide for this repo doesn't use semicolons, so doing the recommended destructuging assignment on an already defined variable causes issues:

({ server, serverAddr } = await spawnGoLibp2p())

The parser appends this to the setTimeout() function call above it (even with an extra line break), resulting in a TypeError: TypeError: testInfo.setTimeout(...) is not a function.

Adding semicolons resolves the issue but makes the code incompatible with the main js-libp2p styles and one that Aegir expects in lints.

I'm going to leave as is.

More then fair enough. Sorry for the trouble 😰

GlenDC · 2023-01-31T20:10:02Z

examples/browser-to-server/tests/test.js

+  })
+
+  play.beforeEach(async ({ servers, page }) => {
+    console.log(`http://localhost:${servers[0].port}/`)


When I log and use such a composed value I usually assign it to a const first, but no biggie

I like this suggestion ❤️ It's implemented.

examples/browser-to-server/tests/test.js

MarcoPolo · 2023-01-31T20:36:40Z

examples/browser-to-server/package.json

    "it-pushable": "^3.1.0",
+    "js-libp2p-webrtc": "../../",


Suggested change

"js-libp2p-webrtc": "../../",

"@libp2p/webrtc": "../../",

ddimaria added 2 commits January 30, 2023 16:46

Add playwright tests and add to CI

334cda3

Install deps

6cf38d4

ddimaria requested review from MarcoPolo, achingbrain and ckousik January 31, 2023 00:05

MarcoPolo suggested changes Jan 31, 2023

View reviewed changes

examples/browser-to-server/index.js Outdated Show resolved Hide resolved

ckousik reviewed Jan 31, 2023

View reviewed changes

examples/browser-to-server/tests/test.js Outdated Show resolved Hide resolved

ckousik reviewed Jan 31, 2023

View reviewed changes

examples/browser-to-server/tests/test.js Outdated Show resolved Hide resolved

examples/browser-to-server/tests/test.js Outdated Show resolved Hide resolved

ddimaria added 2 commits January 31, 2023 09:51

Replace direct html injection in the example to actually creating dom…

37e9f82

… elements

Small refactors suggested by ckousik

0b8e91f

ddimaria requested a review from MarcoPolo January 31, 2023 17:38

GlenDC reviewed Jan 31, 2023

View reviewed changes

examples/browser-to-server/package.json Outdated Show resolved Hide resolved

GlenDC reviewed Jan 31, 2023

View reviewed changes

examples/browser-to-server/package.json Outdated Show resolved Hide resolved

GlenDC reviewed Jan 31, 2023

View reviewed changes

examples/browser-to-server/tests/test.js Show resolved Hide resolved

MarcoPolo reviewed Jan 31, 2023

View reviewed changes

ddimaria added 3 commits January 31, 2023 16:11

Implement Glen's suggestions

77e5216

Add version to @libp2p/webrtc

a4df900

Update example import to use @libp2p/webrtc

f73edf7

ddimaria requested a review from MarcoPolo February 2, 2023 00:25

MarcoPolo approved these changes Feb 2, 2023

View reviewed changes

MarcoPolo merged commit 9313aa0 into main Feb 2, 2023

MarcoPolo deleted the feature/add-examples-to-ci branch February 2, 2023 06:12

p-shahi mentioned this pull request Feb 2, 2023

Test example in CI #86

Closed

Add playwright tests and add to CI #87

Add playwright tests and add to CI #87

Uh oh!

Conversation

ddimaria commented Jan 30, 2023

Uh oh!

MarcoPolo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!