Default queries fix + reset env vars in tests #120
Conversation
src/testing-utils.ts
Outdated
| process.stderr.write = wrapOutput(t.context) as any; | ||
|
|
||
| // Many tests modify environment variables. Take a copy now so that | ||
| // We reset them after the test to keep tests independent of each other. |
There was a problem hiding this comment.
| // We reset them after the test to keep tests independent of each other. | |
| // we reset them after the test to keep tests independent of each other. |
src/testing-utils.ts
Outdated
| // We reset them after the test to keep tests independent of each other. | ||
| // process.env only has strings fields, so a shallow copy is fine. | ||
| t.context.env = {}; | ||
| Object.assign(process.env, t.context.env); |
There was a problem hiding this comment.
| Object.assign(process.env, t.context.env); | |
| Object.assign(t.context.env, process.env); |
Is this the wrong way round? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign says:
The Object.assign() method copies all enumerable own properties from one or more source objects to a target object. It returns the target object.
const returnedTarget = Object.assign(target, source);
There was a problem hiding this comment.
Thanks for catching that blunder. I read the doc but not the parameter names and for some reason assumed the last parameter would be the target.
Fixed now. So I think we must have been setting the environment to empty at the end of each test. So it was still achieving what we wanted, but potentially doing too much.
src/config-utils.test.ts
Outdated
| process.env['GITHUB_WORKSPACE'] = tmpDir; | ||
|
|
||
| // Check that the default behaviour is to add the default queries. | ||
| // In this case if a config file is specified by does not include |
There was a problem hiding this comment.
| // In this case if a config file is specified by does not include | |
| // In this case if a config file is specified but does not include |
src/config-utils.test.ts
Outdated
| }, | ||
| }); | ||
|
|
||
| // Just create a generic config object with non-default values for all fields |
There was a problem hiding this comment.
| // Just create a generic config object with non-default values for all fields | |
| // Just create a generic config object with a non-default value for one of the fields |
In the other place where you use this comment, you specify way more fields, hence my suggested change.
There was a problem hiding this comment.
Thanks. That comment was copy-pasted and I didn't spot it was now incorrect. I've updated to say the actual important points of this config.
|
Note I've also run some tests in a private repo with a config that triggers the bug. I've checked that I can reproduce it, and that it's fixed both by reverting the previous PR, and by merging this PR. |
robertbrignull
force-pushed
the
default_queries_fix
branch
from
4504239 to
315a9f4
Compare
This primarily fixes a bug from #109 where we were not running the default queries in certain cases and this was causing workflows to fail. If a config file was provided but
disable-default-querieswas not in that config file then we were not running the default queries.I've added a unit test to catch this case. I considered adding an integration test too but wasn't sure if that was overkill and a waste of resources given how much longer the integration tests take to run.
While writing the unit test, I discovered another issue that I have also included in this PR. Many of the unit tests set environment variables because this is how action input values are controlled. These values were persisting between tests and causing interference. In this we I add some code to reset environment variables between tests, and add in all the necessary declarations to get the tests to pass again. I could pull this part out to a separate PR if that would be preferable, though the two PRs would conflict.
Merge / deployment checklist