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

Run JS SDK in separate tabs rather than browsers #108

Closed
wants to merge 6 commits into from

Conversation

kegsay
Copy link
Member

@kegsay kegsay commented Jul 5, 2024

Fixes #107 and should be faster to run. The main benefit here is that we aren't relying so much on extracting the WS URL as this happens on browser startup, which has proven to be flakey.

JS only runs:
Before: ~6m10s to run.
After: ~5m50s.

Full JS/Rust combo tests:
Before: ~10m45s to run.
After: ~10m30s to run.

@kegsay kegsay marked this pull request as draft July 5, 2024 16:38
@kegsay kegsay marked this pull request as ready for review July 8, 2024 08:24
@kegsay kegsay requested a review from richvdh July 8, 2024 08:27
@kegsay kegsay changed the title WIP: Run JS SDK in separate tabs rather than browsers Run JS SDK in separate tabs rather than browsers Jul 8, 2024
internal/api/js/chrome/chrome.go Outdated Show resolved Hide resolved
Comment on lines 34 to 40
type Browser struct {
BaseURL string
Cancel func()
Ctx context.Context // topmost chromedp context
ctxCancel func()
execAllocCancel func()
}
Copy link
Member

Choose a reason for hiding this comment

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

document all these?

@@ -75,3 +75,34 @@ func (b *Browser) Close() {
b.ctxCancel()
b.execAllocCancel()
}

func (b *Browser) NewTab(baseJSURL string, onConsoleLog func(s string)) (*Tab, error) {
Copy link
Member

Choose a reason for hiding this comment

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

document?

internal/api/js/chrome/browser.go Outdated Show resolved Hide resolved
Comment on lines 56 to 60
// make a Chrome browser
browser, err := GlobalBrowser()
if err != nil {
return nil, err
return nil, fmt.Errorf("GlobalBrowser: %s", err)
}
Copy link
Member

Choose a reason for hiding this comment

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

To confirm: these changes are nothing to do with the subject of the commit, right?

Comment on lines 84 to 112
// strip /dist so /index.html loads correctly as does /assets/xxx.js
c, err := fs.Sub(jsSDKDistDirectory, "dist")
if err != nil {
return nil, fmt.Errorf("failed to strip /dist off JS SDK files: %s", err)
return nil, fmt.Errorf("failed to create new js sdk instance: %s", err)
}

baseJSURL := ""
// run js-sdk (need to run this as a web server to avoid CORS errors you'd otherwise get with file: URLs)
var wg sync.WaitGroup
wg.Add(1)
mux := &http.ServeMux{}
mux.Handle("/", http.FileServer(http.FS(c)))
srv := &http.Server{
Addr: fmt.Sprintf("127.0.0.1:%d", listenPort),
Handler: mux,
}
startServer := func() {
ln, err := net.Listen("tcp", srv.Addr)
if err != nil {
panic(err)
}
baseJSURL = "http://" + ln.Addr().String()
fmt.Println("JS SDK listening on", baseJSURL)
wg.Done()
srv.Serve(ln)
fmt.Println("JS SDK closing webserver")
}
go startServer()
wg.Wait()
Copy link
Member

Choose a reason for hiding this comment

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

ugh it may be my covid-addled brain but I'm still struggling to follow this a bit. It still feels like this is mixing up refactoring and functional changes. Can't you make this easier for the reviewer to follow?

Sorry, I'm going to abort reviewing this, for now at least

kegsay and others added 2 commits July 9, 2024 07:43
Co-authored-by: Richard van der Hoff <[email protected]>
Co-authored-by: Richard van der Hoff <[email protected]>
@kegsay
Copy link
Member Author

kegsay commented Sep 30, 2024

Closing on the basis this is causing test flakes.

@kegsay kegsay closed this Sep 30, 2024
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.

Check usage of chromedp
2 participants