Skip to content

Add tests for config-utils #36

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 18, 2020
Merged

Add tests for config-utils #36

merged 2 commits into from
May 18, 2020

Conversation

robertbrignull
Copy link
Contributor

@robertbrignull robertbrignull commented May 14, 2020

Adds tests for the config-utils file. This does change the behaviour in a couple of minor cases, but largely this PR is trying to just assert the current behaviour.

I'm not a huge fan of the fact that we silently ignore invalid yaml fields in certain cases. I'd prefer it if we always threw an error, but I didn't want to make that change here. If we decide to do that in the future then I'd feel much happier having tests of this functionality.

A side note, I had to add the --serial flag when running the tests to stop running them in parallel. My understanding (according to the docs) is that tests from separate files are run in separate nodejs processes, but tests within a file are run in one process. Therefore since we are messing with process environment variables in order to control our code we need to run tests serially and avoid parallelism.

Merge / deployment checklist

  • Run test builds as necessary. Can be on this repository or elsewhere as needed in order to test the change - please include links to tests in other repos!
    • CodeQL using init/analyze actions
    • 3rd party tool using upload action
  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.

@robertbrignull
Copy link
Contributor Author

The code scanning check failure is because there's 1 new alert and 1 fixed alert because I changed the line where the alert appears. Nothing to worry about.

@robertbrignull
Copy link
Contributor Author

Also note this PR includes some but not all of the changes from #22

@robertbrignull robertbrignull merged commit ff40939 into master May 18, 2020
@robertbrignull robertbrignull deleted the config_utils_tests branch May 18, 2020 14:42
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.

2 participants