Test options make feature report too big

[Fryuni]

When recording a run, after each file, Cypress sends the test suite result to the server along with the test configuration. When using this plugin, complex feature files result in enormous payloads when reporting to the server.

The entire Pickle is passed as part of the environment on the test config here:

cypress-cucumber-preprocessor/lib/create-tests.ts

Lines 314 to 334 in 755c6f0

	const internalProperties: InternalSpecProperties = {
	pickle,
	testCaseStartedId: uuid(),
	allSteps: steps,
	remainingSteps: [...steps],
	};
	const internalEnv = { [INTERNAL_SPEC_PROPERTIES]: internalProperties };
	const suiteOptions = tags
	.filter(looksLikeOptions)
	.map(tagToCypressOptions)
	.reduce(Object.assign, {});
	if (suiteOptions.env) {
	Object.assign(suiteOptions.env, internalEnv);
	} else {
	suiteOptions.env = internalEnv;
	}
	it(scenarioName, suiteOptions, function () {

Cypress serializes the entire test options for each test when reporting to the recording server, batching all the reports from a single file into one request. This causes the server to error or to timeout on complex features.

One of our features is described in a file with 270 lines (not much) and has 31 scenarios (also not much), but it is somewhat complex with multiple tables, rules, and outlines. The payload to report its result is over 22MiB, reporting to both Cypress Cloud, Currents, and Sorry-Cypress fails with error 500, 413 or times out after 30 seconds.

[Fryuni]

I have already investigated this and have a working proposal.

At first, I intended to open an issue on Cypress itself to allow configuring a test not to report its configuration. I tried that locally by just excluding the parameter and it breaks multiple functionalities of Cypress Cloud.

I then tried changing this plugin so it would not rely on the test options, but it becomes so much messier than it is now to pass data between the plugin, the tests, and the hooks that I don't think would be a good idea either.

The minimal solution I found was to keep everything where it is, using the environment in the test options and Cypress.env, but to hide the internal properties of the plugin from the recording. Since those values should only ever be used internally, sending them anywhere is unnecessary.

We can change this line:

cypress-cucumber-preprocessor/lib/create-tests.ts

Line 321 in 755c6f0

const internalEnv = { [INTERNAL_SPEC_PROPERTIES]: internalProperties };

To this:

  const internalEnv = {
    [INTERNAL_SPEC_PROPERTIES]: internalProperties,
    toJSON() { return null; },
  };

The internal properties are converted to null when Cypress serializes the options to send to the server.

Testing on our local cases, the feature files causing over 20MiB payloads now generate about 150KiB. Reporting to any of the servers mentioned in the issue works without any problems.

Since this change of overwriting the serialization of a single instance is, AFAICT, not a very common practice within the JS community, I'm sending this as an issue first for discussion.

What do y'all think?

[badeball]

Hey @Fryuni, interesting problem you're describing here, I had no idea about this.

I then tried changing this plugin so it would not rely on the test options, but it becomes so much messier than it is now to pass data between the plugin, the tests, and the hooks that I don't think would be a good idea either.

It quickly becoming messy would be my suspicion as well. Plugin state is unfortunately a bit of a though nut. Because it's so complex, it's imperative IMO that any requirements are adequately tested.

What do y'all think?

Provided that this approach works, I imagine it would also wipe out user-defined environment values from tags.

Furthermore, how does it affect values specified through the config file?

I suspect the approach relies on the method being merged with all other sources of env values and ultimately override the serialization. Will then any env values be serialized?

[Fryuni]

Provided that this approach works, I imagine it would also wipe out user-defined environment values from tags.

Furthermore, how does it affect values specified through the config file?

I suspect the approach relies on the method being merged with all other sources of env values and ultimately override the serialization. Will then any env values be serialized?

Indeed it merges the values and doesn't serialize any of the environment variables, even from the config file.
The environment variables from the config file are still present on the Cypress Cloud dashboard even while blocking all the env variables since it sends another request for the whole test suite with the environment from the config. That is probably being merged on the server.

The environment variables set from the tags don't get reported, which could be a problem for people debugging if they look for it.

Adding the toJSON function to the internalProperties instead of the internalEnv sends all the other environment variables and includes a null for the internal properties, which also works.

We've tested it with our suite locally reporting to the server (by editing the file in node_modules), and it worked perfectly. We'll make a fork, run all the tests on our CI pointing to the fork, and send an update here.

[badeball]

Adding the toJSON function to the internalProperties instead of the internalEnv sends all the other environment variables and includes a null for the internal properties, which also works.

This would definitely be my preferred approach here.

We've tested it with our suite locally reporting to the server (by editing the file in node_modules), and it worked perfectly. We'll make a fork, run all the tests on our CI pointing to the fork, and send an update here.

I can recommend using patch-package, it'll save you some trouble.

[Fryuni]

We've tested it with our suite locally reporting to the server (by editing the file in node_modules), and it worked perfectly. We'll make a fork, run all the tests on our CI pointing to the fork, and send an update here.

I can recommend using patch-package, it'll save you some trouble.

I didn't know about this. It's awesome 🤩, thanks!

Adding the toJSON function to the internalProperties instead of the internalEnv sends all the other environment variables and includes a null for the internal properties, which also works.

This would definitely be my preferred approach here.

Everything is working on our test suite now. I'll open a PR.

Do you think this should be opt-in/opt-out in case there is someone relying on those properties being on the report or should it always be hidden?

[badeball]

Do you think this should be opt-in/opt-out in case there is someone relying on those properties being on the report or should it always be hidden?

Opt-out for hiding, IE. hiding by default would be my preference. These are internal properties which no one should really be relying on, as the property name suggest.

[Fryuni]

Okay, I'll try to send the PR this year, but if I don't:

Happy new year 🎉 🥳

[badeball]

I've been thinking a little bit further on this and I think overriding methods is unnecessary. One of the hurdles with state is ensuring that state is available anywhere within the test, including beforeEach hooks. Hence we declare it using Cypress environment variables and let Cypress deal with it.

However, it seems to me that the only thing that's really needed is a reference. The (big) data might very well be contained in a different Map / WeakMap and accessed using the current test's reference when needed.

Thus I think we can possibly moving almost everything away from Cypress environment and avoid filling it up with a lot of internal data.

[Fryuni]

Took me quite a while to get the tests to run locally, tests for Cypress plugins are a pain to run on NixOS.

Just opened the PR, let me know what you think.

[badeball]

Fixed and released as v15.1.2.

[MDG-JHowley]

I realise it's an odd use case but I was actually relying on the INTERNAL_SPEC_PROPERTIES values. We use feature flags and test across multiple environments, sometimes a test is written against a specific flag state so I have added in a custom tag to our tests with the format: @flag('someFlagKey','true') where the 1st value is the flag name and the 2nd is the required state.

When our tests run there is a Before hook to check any required flags in the tags and automatically skip them if the target environment has the flag off. Otherwise we would just have potentially long-running and failing tests

The crux of the code being something like:

const {
  INTERNAL_SPEC_PROPERTIES,
} = require('@badeball/cypress-cucumber-preprocessor/lib/constants');

const scenarioTags = () => {
  return Cypress._.map(
    Cypress._.get(Cypress.env(INTERNAL_SPEC_PROPERTIES), 'pickle.tags', []),
    'name'
  );
};

Then some parsing of the tags after the fact.

---

With version v15.1.2 of this plugin I just get back the reference instead. And my alternative attempts just thrown an error e.g.

const {
  isFeature,
} = require('@badeball/cypress-cucumber-preprocessor/lib/methods');
const createTests = require('@badeball/cypress-cucumber-preprocessor/lib/create-tests');

console.log({
    isFeature: isFeature(),
    pickle: createTests.retrieveInternalSpecProperties(),
  });

Which results in:

Expected to find internal spec properties with reference = 9c070b3e-e3ef-4088-9925-b5be1f9f4e9c (this might be a bug, please report at https://github.com/badeball/cypress-cucumber-preprocessor)

isFeature() returns true BTW

@badeball do you feel this is a valid use-case? Should I raise a separate issue?

[Fryuni]

Also, @badeball, I don't see where the values inside the Map would ever be dropped. Won't this map grow unbounded with the total number of tests?

The WeakMap used before would not prevent anything from being collected.

[badeball]

Hey @MDG-JHowley, check out testState as illustrated here. I'm working on a more appropriate API for this, but use the global variable for now.

[badeball]

@Fryuni, you're right that they're not dropped, but they only live for the duration of the current spec that is open, as far as I've understood. Furthermore, almost all values are just reference pointers to the same GherkinDocument and Pickle's, which would be prevented from being collected anyway due to being put into the window object. In other words, I don't think this actually entail increased memory usage, nor memory leak, but tell me if you observe otherwise.

[Fryuni]

We haven't updated the library yet, but I'll add a note to the task to be sure we pay attention to this point.

If it is all references to the same object it is probably fine, I'll open a new issue otherwise.

[badeball]

Cypress serializes the entire test options for each test when reporting to the recording server, batching all the reports from a single file into one request. This causes the server to error or to timeout on complex features.

@Fryuni, do you know where in the Cypress source code that this serialization takes place?

[Fryuni]

Cypress serializes the entire test options for each test when reporting to the recording server, batching all the reports from a single file into one request. This causes the server to error or to timeout on complex features.

@Fryuni, do you know where in the Cypress source code that this serialization takes place?

I don't.

I added a proxy before our Sorry Cypress instance to save every request to a file. That is how I discovered it was being serialized, but I haven't looked deep into the code to know why.

[badeball]

I have changed this with v17.2.1 and went with a "overide toJSON" approach, as seen in
475f4ad. The reason for this was, as explained in the commit, that doesFeatureMatch broke (#1025). Let me know if there are any issues with the report.

[Fryuni]

Thanks for the heads up, I'll ask the team to do a trial run with the latest version to see if anything breaks.

[Fryuni]

Just run our entire test suite with the latest version. Everything is working great, even the ginormous test cases.

So far no problem, but if anything happens with the new tests coming I'll open a new issue.

[badeball]

Excellent, thanks for getting back to me so quickly. :)