WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
9.99k stars 4.02k forks source link

e2e-test-utils-playwright - RequestUtils - add support for extraHTTPHeaders #62466

Open Chrico opened 3 weeks ago

Chrico commented 3 weeks ago

What problem does this address?

Currently the RequestUtils::setup() creates a new Context and only uses baseURL and storageState (by resolving the storageStatePath), but it does not support further options like extraHTTPHeaders or httpCredentials.

What is your proposed solution?

It would be good to enhance the RequestUtils::setup()-method by splitting up the paramaters into "browser context options"-object to support all possible options and the rest (like user) is separated.

So instead we could change it to following:

interface BrowserContextOptions extends Readonly< {
    acceptDownloads?: boolean,
    baseURL?: string,
    bypassCSP?: boolean,
    colorScheme?: null | "light" | "dark" | "no-preference",
    deviceScaleFactor?: number,
    extraHTTPHeaders?:  { [ key: string ]: string },
    forcedColors?: null | "active" | "none",
    geolocation?: {
        latitude: number,
        longitude: number,
        accuracy?: number
    },
    hasTouch?: boolean,
    httpCredentials?: {
        username: string,
        password: string,
        origin?: string
    },
    // snip - and so on ...
    // @link https://playwright.dev/docs/api/class-browser#browser-new-context
}> {}

class RequestUtils {

   // snip

    static async setup( context?: BrowserContextOptions, user?: User, storageStatePath?: string ) {
       // snip
    }
}

See full list of Browser Context Options: https://playwright.dev/docs/api/class-browser#browser-new-context

kopepasah commented 3 days ago

Currently, we have a test site that uses httpCredentials for access, and we are unable to pass any additional options to the setup() method. I agree with @Chrico that these options must be added, and the best case is to pass all available options to Playwright, rather than limit the options available with the WordPress E2E library.

Generally speaking, when a abstraction library is created, it is a good practice to allow passing of any additional options to the underlying library (unless there is a specific reason not to do so).

Does anyone know if there was a specific reason for not allowing all options to be passed (asking just in case there is a caveat to this approach)?

kopepasah commented 3 days ago

See full list of Browser Context Options: https://playwright.dev/docs/api/class-browser#browser-new-context

@Chrico I think we need to pass the https://playwright.dev/docs/api/class-apirequest#api-request-new-context, correct? Very similar to browser context options, but specific to the APIRequest class.

Chrico commented 2 days ago

@Chrico I think we need to pass the https://playwright.dev/docs/api/class-apirequest#api-request-new-context, correct? Very similar to browser context options, but specific to the APIRequest class.

Yes, you're right, the RequestUtils is not using the Browser and instead the APIRequest. 👍🏻