jwplayer / ott-web-app

Reference implementation for JWP-powered apps
Apache License 2.0
70 stars 52 forks source link

Test / fix e2e overlapping reports #495

Open ChristiaanScheermeijer opened 5 months ago

ChristiaanScheermeijer commented 5 months ago

Description

Because we use a function to wrap the JWP and Cleeng scenarios, the CodeceptJS run workers' reporting fails. The reporter instance exists only once for each worker and it creates two Allure runs within the same instance because of the duplicate Feature(`subscription - ${providerName}`) calls.

I've played with data-driven tests, but this will run all steps sequentially.

The easiest fix was to either remove the Feature and Before calls from the runTestSuite functions so it only creates 1 test run.

After this change, the Allure reporting will show all the scenarios before this was the last one completed.

image

https://github.com/codeceptjs/allure-legacy/blob/8c31bd2d0736487edaa7d92a813562b2571cef49/index.js#L180-L182

Calls:

Allure.prototype.startSuite = function(suiteName, timestamp) {
    this.suites.unshift(new Suite(suiteName, timestamp));
};
Allure.prototype.endSuite = function(timestamp) {
    var suite = this.getCurrentSuite();

    suite.end(timestamp);
    if(suite.hasTests()) {
        writer.writeSuite(this.options.targetDir, suite);
    }
    this.suites.shift();
};

The culprit... This function only returns the first suite:

Allure.prototype.getCurrentSuite = function() {
    return this.suites[0];
};
github-actions[bot] commented 5 months ago

Visit the preview URL for this PR (updated for commit d8f0b21):

https://ottwebapp--pr495-fix-e2e-overlapping-on02e5ih.web.app

(expires Sat, 11 May 2024 11:58:55 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: c198f8a3a199ba8747819f7f1e45cf602b777529

ChristiaanScheermeijer commented 5 months ago

I was playing with a different idea.

It is possible to customise the parallel execution of tests in CodeceptJS. Creating a folder for all integration tests and running these with different configurations would be a nice and scalable solution.

These suites will have to be split for two integration options:

const integrations = ['jpw', 'cleeng'];

// Create two configs and inject the integration (name for now)
const configs = integrations.map((integrationName) => ({
  inject: {
    integrationName,
  },
}));

// for each integration, spawn a worker and register the test group containing all integration tests 
// can also be split over multiple workers...
for (const config of configs) {
  const worker = workers.spawn();
  worker.addTests(testGroups[0]);
  worker.addConfig(config);
}

The rest of the files are spread over multiple workers like normal and don't need any special configuration.

dbudzins commented 5 months ago

I was playing with a different idea.

@ChristiaanScheermeijer This looks promising and more maintainable to me. Any downsides?

ChristiaanScheermeijer commented 5 months ago

@ChristiaanScheermeijer This looks promising and more maintainable to me. Any downsides?

I was playing with this yesterday and I got it (almost) working. The run workers script does feel a bit too custom because I have to trick codecept into duplicating the integration tests with a different config.

The biggest downside is that the e2e tests will only test 1 integration when you run them via the UI or with steps (easier to debug). This is because the run-workers script changes the codecept config before spawning them.

This is what I got so far:

I created two integration configurations because injection works via require and no inline objects :')

image

The codecept config can either have a predefined include, or we need to ignore the tests by default (when running steps or UI):

    },
  },
  include: {
    I: './utils/steps_file.ts',
    integration: `./configs/jwp`,
  },
  bootstrap: null,

The run-workers script creates two config groups with overrides:

// Create two configs and inject the integration (name for now)
const configs = integrations.map((integrationName) => ({
  include: {
    integration: `./configs/${integrationName}`,
  },
}));

Currently, 4 workers are spawned, but this needs to be worked out a bit more:

for (const config of configs) {
  for (const group of testGroups) {
    const worker = workers.spawn();
    worker.addTests(group);
    worker.addConfig(config);
    worker.addOptions({ config: __dirname + '/codecept.desktop.ts', verbose: true });
  }
}

The challenge here is to balance everything out. If we only create two workers for the integrations, the rest of the tests are distributed over the remainder of the workers.

However, we could also run the non-integration tests on the integration-configured worker...

ChristiaanScheermeijer commented 5 months ago

@dbudzins do you think this idea still has potential? We are looking into optimizing the e2e tests (duration), but this might change based on this outcome.

dbudzins commented 5 months ago

@dbudzins do you think this idea still has potential? We are looking into optimizing the e2e tests (duration), but this might change based on this outcome.

Hm, it's getting a bit fiddly now.... it looks like just splitting the files fixes the reporting issue. It's kinda annoying, but seems like a quick easy fix: https://github.com/jwplayer/ott-web-app/pull/503

For whatever reason it also looks like they still won't run in parallel, but since there are a lot of tests to run, I think this will shake out in the wash.

What do you think?

ChristiaanScheermeijer commented 5 months ago

@dbudzins As of now, I'm doubting between keeping it "as is" or proceeding with #503. I can pick up on #503, submit a PR and close this one?

AntonLantukh commented 5 months ago

@ChristiaanScheermeijer any preference here?