Chore: replace spectron with playwright

[jackkav]

Related: electron-userland/spectron#1045

The primary goal of this PR is to unblock our electron upgrade PR #4232 which is blocked by spectron flaking out on windows and blocking future upgrades due to its deprecation.

Improvements

• less flake in the playwright attaching to window mechanism
• runs twice as fast
• fails fast when missing build artifact

Scoped out

• screenshot/tracing on fail
• test writing guidance
• retry logic
• videos

Notes:

• windows doesn't start properly so theres a forced reload in the test setup, this should be investigated separately
• locking to single instance doesn't work in playwright, this should be investigated separately
• users who may have been using the undocumented env var INSOMNIA_DATA_PATH may get logged out because of the setPath fallback used for controlling migration context. broke this out into a separate issue

Considered in this scope

• docs, designed initially to be operated as spectron was
• screenshot/video on fail, going to delay into a PR that adds the more powerful tracing support
• locking issue, going to delay into an investigation
• test setup fixtures, going to delay into the tracing PR above
• better insomnia request collection fixture, not required
• decide on playwright config, defaults are okay for now
• decide if this PR should include code re-use abstraction, not yet

Closes INS-1300
Closes INS-1147

[jackkav]

Thank you for your thoughts and concerns I hear you and hope to address them to your satisfaction.

I know you've chosen to descope writing utilities/page objects from this PR. In theory that's fine, it's a work in progress, but we're deleting a lot of valuable code and if we slightly alter the approach we can take a step up from Spectron instead of start from scratch. Note that what we are changing is the test framework, and from what I've seen in this PR that doesn't need to involve writing things from scratch.

Playwright and Spectron are not a 1:1 API match, there are new playwright specific conventions to learn and adapt, and it is unwise to learn and adapt to them as one contributor in a single PR.

The existing page object approach of writing an e2e test is agnostic of the framework being used. If you look at the overall functions - click a request, click send, click a tab, open a dropdown - and how they are parameterized, they describe user actions, and the implementation describes how the test framework should handle those user actions.

The reason this code uses playwright recordings for its runs are to separate any of my personal opinions about test structuring from the replacement effort, while creating an opportunity to revisit existing conventions, I agree the modules were very useful in making readable spectron tests possible, and we will certainly want to pull in some of the ideas with playwright. However I strongly believe the introduction of abstractions over test structure should happen carefully as these tests have a huge impact on the contribution experience, so we have a duty to keep them as lean and fast as possible to maximise return on investment.

We've changed our test framework from Spectron to Playwright. Updating the page object implementations to work with Playwright's API will allow us to keep the existing tests (which are much more readable than the what I'm reviewing in this PR) and continue from there.

Readability is subjective and not something I aim to improve in this PR, only to provide us with a starting point to build confidence in our e2e testing.

I have a strong opinion that it is not necessary to delete all of the existing tests and page objects and start again. This is feedback about the overall approach, please don't take it as a request to increase the scope of this PR. How can we help ourselves build on top of what's already written, instead of writing from scratch?

We can build on top by writing new tests ourselves on top of this minimal safety net, to show the value of required abstractions.

My goal is to eliminate barriers to contribution, to me this means making it intuitive, easy to change and with as little indirection as possible.

[filfreire]

Marking this as approved, seems to work ok, just some minor comments:

• Tests were passing earlier now are failing, I suspect due to a missing click on the can send requests scenario
• Left some notes of stuff we might want to look at in the future - like spinning up gRPC and Websocket test servers like you did for http, and also we might want to explore tests where we purposely import insomnia data folder that are in a weird state.

[filfreire]

@jackkav by removing this the tests seem to be failing now, since playwright can't open the collection to try and issue the requests.

[DMarby]

Can confirm this locally as well, adding this line back in makes the test pass for me

[jackkav]

I was seeing import from clipboard go straight to the send request view, so I assumed it had been changed in a recent PR after rebase. Not sure though.

[filfreire]

To pick on something @dimitropoulos had mentioned a while back, I think we could just try and also change the commands both here and in the package.json to something that is more readable and simpler.
Related to this ticket: https://linear.app/insomnia/issue/INS-1276/improve-npm-script-experience

Perhaps reducing all our smoke commands to just 2, something like npm run smoke:build and npm run smoke:test? (Not sure about the naming though)

[jackkav]

I like it!

[jackkav]

It's in so many places in the code base and meant to exist alongside many other contexts so I'll scope it out for now, but worth a later PR to look for ways to clarify the wording of app-build:smoke and test:smoke:build

[develohpanda]

I agree with this to try and increase clarity! There are a few variations hence the confusion:

• build the app
• build the app for smoke tests
• package the app
• package the app for smoke tests
• run smoke tests against build artifacts
• run smoke tests against the packaged binaries

Building the app is faster than packaging the app. But certain things (such as testing plugin installation) requires the test to be run against a packaged binary instead of build artifacts. Testing against the packaged app is also more realistic, so it's just weighing up the pros and cons.

Additionally, the first four bulletpoints can become two bulletpoints if we can remove/address the requirement for a gitref in the build script:

insomnia/packages/insomnia-app/scripts/getBuildContext.ts

Lines 38 to 51 in 5f4c19d

	export const getBuildContext = (forceFromGitRef: boolean) => {
	if (forceFromGitRef) {
	return fromGitRef();
	}
	if (process.env.SMOKE_TEST) {
	return {
	smokeTest: true,
	version: '0.0.1',
	} as const;
	}
	return fromGitRef();
	};

[filfreire]

Same opinion as @develohpanda on this, this is awesome. There's probably a way for us to mimic this and add one for gRPC, and later on websockets , etc. Really cool stuff 👍

[filfreire]

This is giving me an idea - let's say we get a bug that is only reproducible with someone's backup of a data folder, we could make it so folks can point to a data path for the test they want to run - which we would fetch locally or maybe download from S3 bucket where we store weird data folder backups.

[filfreire]

I'm not sure if my other comment will be visible, but here we're missing the step before this one where we click to open the collection after importing.

[DMarby]

Awesome 👍

Found two remaining references to Spectron, not sure if we have the same issue with the tsconfig with Playwright as we did for Spectron?

insomnia/packages/insomnia-smoke-test/tsconfig.build.json

Line 9 in b2c94eb

"skipLibCheck": true, // this is required because spectron depends on electron but it is not locatable by typescript for the purpose of types

insomnia/DEVELOPMENT.md

Line 60 in b2c94eb

to write our unit tests, and [Spectron](https://www.electronjs.org/spectron) for integration tests.

[develohpanda]

Thanks for your thoughts as well! Readability is subjective but readability helps with code reviews, and while the contribution experience is important, maintainability is also important when tests break and someone goes in with fresh eyes, and over time I hope we can reach a balance between all of these factors.

One action item: could you please make follow-up tasks to go to the existing smoke tests and migrate many of the assertions across?

For example, the PDF viewer assertion, or the XML filter assertion are both regression tests and we shouldn't lose those learnings.

insomnia/packages/insomnia-smoke-test/core/app.test.ts

Lines 81 to 94 in b2c94eb

	await debug.expect200(app);
	const responseViewer = await debug.getResponseViewer(app);
	const partialExpectedResponse = '<LoginResult>xxx-777-xxx-123</LoginResult>';
	await debug.expectContainsText(responseViewer, partialExpectedResponse);
	await debug.typeInResponseFilter(app, "//*[local-name(.)='LoginResult']/text()");
	await waitUntilTextDisappears(app, responseViewer, partialExpectedResponse);
	await debug.expectContainsText(
	responseViewer,
	'<result>xxx-777-xxx-123</result>',
	);

.

Another example is the Basic Auth test. Instead of simply testing basic auth works through a header, it's purpose is to use the Auth tab within Insomnia to simulate a user typing into the fields, enabling and disabling auth and verifying the expected response.

tl;dr through this PR we lose a lot of coverage, and we should make an effort to bring across the existing tests once this has merged to reinstate that coverage, hence follow-up tasks.