bahmutov / cypress-split

Split Cypress specs across parallel CI machines for speed
MIT License
199 stars 22 forks source link

Align config.env with ENV variables #192

Closed OriR closed 6 months ago

OriR commented 6 months ago

Thank you for this amazing plugin 🙏

I encountered something that I feel is a bug - config.env attributes don't behave the same as ENV variables. Here are a few examples:

  1. let SPLIT_OUTPUT_FILE = process.env.SPLIT_OUTPUT_FILE || config.env.outputFile || SPLIT_FILE - in this case the ENV variable name is SPLIT_OUTPUT_FILE but the one in config.env is outputFile which is odd (missing the "split" prefix).
  2. The SPLIT_INDEX variable is a bit more buggy because of this:
    if (process.env.SPLIT_INDEX1 || config.env.splitIndex) {
    ...
    }

    So using config.env.splitIndex technically means you're always doing a 1-based index (unless you pass a number and for the 0th index it'd be 0-based but for the rest it'd be 1-based) while SPLIT_INDEX env variable defaults to 0-based index.

I would assume that for every env variable VAR_NAME there would be a corresponding attribute in config.env that behaves the same but the above 2 examples don't support this idea, so I don't know if it's by design or a mistake.

Anyway, thanks for the plugin, just thought I'd share this since I haven't seen anyone mentioning it.

bahmutov commented 6 months ago
  1. fixing the env name and will release a fix when I merge #194
  2. I don't understand the problem you are describing. Of course, there might be a problem, but can you please provide which env variable inputs you are using, what you assume the plugin does, and what you assume it should do instead? Tip: you can enable debug logs to see how the env variables are parsed https://github.com/bahmutov/cypress-split?tab=readme-ov-file#debugging
github-actions[bot] commented 6 months ago

:tada: This issue has been resolved in version 1.17.2 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

OriR commented 6 months ago

@bahmutov Apologies if I wasn't clear. What I meant by No.2 is this - Let's say I have this config:

export default defineConfig({
    e2e: {
        env: {
            splitIndex: process.env.MY_CI_SPLIT_INDEX,
            split: process.env.MY_CI_SPLIT,
        },
        setupNodeEvents(on, config) {
            require("cypress-split")(on, config);
            return config;
        },
    },
});

I'm using Jenkins with an in-house parallelization mechanism where it sets these env variables MY_CI_SPLIT_INDEX and MY_CI_SPLIT (I renamed them a bit for clarity) which correspond to SPLIT_INDEX and SPLIT. Instead of mapping them to other env variables, I'm using the env config in cypress.config.ts to map them, which is pretty simple.

Now, because of these lines: https://github.com/bahmutov/cypress-split/blob/3c8058a96a398b568784c6ef6daeb4e765842312/src/index.js#L98-L109

config.env.splitIndex would be defined (since it's in my config) it thinks it's a 1-based index and enters the if-block, however if I were to set SPLIT_INDEX through an env variable it wouldn't think it's a 1-based index but rather 0-based. This, to me, feels like config.env.splitIndex and SPLIT_INDEX are not aligned on this behavior. I would expect both to work the same way and be 0-based since there's a special variable for 1-based (SPLIT_INDEX1).

The way to solve it would be to do this:

- if (process.env.SPLIT_INDEX1 || config.env.splitIndex) { 
+ if (process.env.SPLIT_INDEX1 || config.env.splitIndex1) { 

Use a different attribute in the config for 1-based index.

bahmutov commented 6 months ago

yeah, makes sense, will release a fix soon.