Skip to content

Conversation

NSeydoux
Copy link
Contributor

This changes the environment validation so that booleans are used to flag env vars that should be validated, instead of a value of their type. Meaning

getBrowserTestingEnvironment({
      clientCredentials: {
        owner: { login: "", password: "" },
      },
    });

becomes

      clientCredentials: {
        owner: { login: true, password: true },
      },
    });
```, and the underlying environment variable get type-checked.

Environment validation had a design flaw resulting in environments not being validated as expected.
This removes some duplication by factorizing common code and using the type system to enforce defaults.
@NSeydoux NSeydoux requested a review from a team as a code owner January 10, 2023 13:00
Copy link
Contributor

@ThisIsMissEm ThisIsMissEm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this may not do what we want, but I may be misreading the code.

if (
vars.clientCredentials?.owner?.id &&
typeof vars.clientCredentials.owner.id !== "string"
typeof process.env.E2E_TEST_OWNER_CLIENT_ID !== "string"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
typeof process.env.E2E_TEST_OWNER_CLIENT_ID !== "string"
typeof process.env.E2E_TEST_OWNER_CLIENT_ID !== "string" && process.env.E2E_TEST_OWNER_CLIENT_ID !== ""

We should be checking that we actually have a valid value set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in db46a19.

@NSeydoux NSeydoux requested a review from ThisIsMissEm January 13, 2023 10:36
Copy link
Contributor

@ThisIsMissEm ThisIsMissEm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This'll do for now, though I was looking at these:

Which might be more maintainable in the future, as the index.ts file is getting a bit unwieldy.

@NSeydoux NSeydoux merged commit 7b5bb9b into main Jan 19, 2023
@NSeydoux NSeydoux deleted the fix/library-vars-boolean branch January 19, 2023 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants