SaveState improvements

[sonic16x]

Changes:

• After the tests finish, the state file will be automatically deleted. To prevent this, pass the keepFile: true option to the - command saveState.
• If the directory specified in the file path does not exist, it will be created automatically.
• If the state object is empty, the file will not be saved.
• add getState command

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/gemini-testing/testplane@1177

commit: 48cb472

[shadowusr]

Great job! 🔥

I've left a couple of suggestions, specifically about configuration options for state* commands. The timeout of 5 mins in tests is a bit concerning as well

If in doubt about the config option, feel free to drop me a PM or discuss the matter in the team chat.

[shadowusr]

Timeout of 5 minutes? This is not normal IMO =(

What's taking so much time?

[sonic16x]

Fixed

[shadowusr]

Hmmm are we sure such invasive error message is needed? I'd leave no message at all...

[sonic16x]

Yea, but this option will be used only for local development and it is good message if you forgot disable it

[shadowusr]

I think the triple exclamation mark is definitely too much !!!. Also, the message should be actionable — what can I do with this message? I think it should explicitly mention that you should disable the "keepFile" option in saveState command.

[shadowusr]

I think introducing this random env variable is a bad idea.

I would suggest introducing the dedicated config section for the state* commands. It could have the following naming and structure:

interface StateOpts {
    cookies?: boolean;
    localStorage?: boolean;
    sessionStorage?: boolean;
    path?: string;
    cookieFilter?: (cookie: Cookie) => boolean;
    refresh?: boolean;
    keepFile?: boolean;
}

interface CommonConfig {
    // ...
    stateOpts: StateOpts;
}

This would cover many points at once:

• It would automatically create env variables, consistent with other config options (testplane_state_opts_keep_file=true) as well as CLI args (`--testplane-state-ops-keep-file=true)
• Users could specify default path and then use getState/saveState/restoreState without args
• Users can set their own defaults to other options
• It's consistent with how we approach configuring other commands in testplane

[sonic16x]

Fixed

[shadowusr]

From the user experience point of view, I've tried using these new features, and experienced major issues:

With this config:

    beforeAll: async function() {
        const browser = await launchBrowser({
            desiredCapabilities: {
                browserName: 'chrome',
                browserVersion: '138.0',
            },
        });
        await browser.url('http://ya.ru/');
        await browser.saveState({
            keepFile: true,
            cookieFilter: (cookie) => cookie.name.includes('ya'),
        });

        await browser.deleteSession();
    },
    
    stateOpts: {
        path: 'browser-state.json',
    },

The file won't save to browser-state.json at all. Looks like stateOpts doesn't merge well with params passed to saveState command.

The other scenario won't work as well:

    beforeAll: async function() {
        const browser = await launchBrowser({
            desiredCapabilities: {
                browserName: 'chrome',
                browserVersion: '138.0',
            },
        });
        await browser.url('http://ya.ru/');
        await browser.saveState({
            path: 'browser-state.json',
            cookieFilter: (cookie) => cookie.name.includes('ya'),
        });

        await browser.deleteSession();
    },

And then in test:

it('snippet testing', async ({browser}) => {
    await browser.url('http://ya.ru/');
    console.log(await browser.getState());
});

And in test the state already doesn't exist. Looks like we remove state file too early.

I suggest to thoroughly test this feature once again from the user point of view.

[shadowusr]

Why do we have process.exit over here and in other state* commands?

I think it is very wrong, because with process.exit there can be no proper cleanup at all. In cases when we need to urgently crash process, the halt should be used, that first tries to perform cleanup:

testplane/src/testplane.ts

Line 296 in 9ed2401

halt(err: Error, timeout = 60000): void {

However, I see no reason at all to crash the whole test run in this case, because it's clearly an issue of a concrete test. Shouldn't we just throw an error over here?

[sonic16x]

Fixed

[shadowusr]

I think the triple exclamation mark is definitely too much !!!. Also, the message should be actionable — what can I do with this message? I think it should explicitly mention that you should disable the "keepFile" option in saveState command.