Skip to content
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

fix(ci) Don't assert against env passed into spawn #1473

Merged
merged 1 commit into from May 5, 2020

Conversation

mrmicahcooper
Copy link
Contributor

When calling psql from a spawned process, we were strictly asserting that all args matched, including our own process.env. So if a test failed, the failed assertion message would dump the env. This removes the assertion of the env and just tests that the proper flags were passed to the psql command.

@mrmicahcooper mrmicahcooper requested a review from a team as a code owner April 9, 2020 19:13
@mrmicahcooper mrmicahcooper changed the title fix(ci) Don't assert against options passed into psql's spawn fix(ci) Don't assert against env passed into spawn Apr 9, 2020
@mrmicahcooper mrmicahcooper self-assigned this Apr 20, 2020
@@ -59,18 +59,9 @@ describe('psql', () => {
})

it('runs psql', () => {
let cp = sandbox.mock(require('child_process'))
Copy link
Contributor

Choose a reason for hiding this comment

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

ensuring the we're passing the right env does seem valuable, and somebody may try to write a similar test in the future.

If we aren't worried about leaking postgres creds, maybe we could assert on a limited subset of env instead of abandoning it entirely?

// setup
const EXPECTED_ENV = Object.freeze({
        PGAPPNAME: 'psql non-interactive'
        PGSSLMODE: 'prefer',	
        PGUSER: 'jeff',	
        PGPASSWORD: 'pass',	
        PGDATABASE: 'mydb',	
        PGPORT: 5432,	
        PGHOST: 'localhost'
});
cp.mock('spawn').callsFake((commandName, args, opts) => {
  expect(commandName).to.equal('psql');
  expect(args).to.deep.equal(['-c', 'SELECT NOW();', '--set', 'sslmode=require']);
  Object.entries(expectedEnv).forEach(([key, expectedValue]) => {
    expect(opts.env[key]).to.equal(expectedValue);
  });
  expect(opts.encoding).to.equal('utf8');
  expect(opts.stdio).to.deepEqual(['ignore, 'pipe', 'inherit']);
  return {
    /* redacted for brevity but everything from `.returns()` goes here */
    stdout: {
    }
  }
}).once();

then we would keep the cp.verify() as before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I gave this a shot and the postgres creds are still hidden (I don't think they matter but still).
verify() doesn't work since that's a method on the mock but I was still able to safely keep the env assertions.

Thanks @fivetanley !

@fivetanley
Copy link
Contributor

fivetanley commented Apr 24, 2020

Overall, it's good to fix this test, but I wonder if we could avoid this for all tests in the future.

If we had some kind of global spec helper that all the plugins could require before mocha starts, we could be paranoid about printing env variables in CI. We could override process.env before any tests runs to make sure that values are scrubbed:

if (process.CI) {
  let originalEnv = process.env;
    const sanitizedEnv = Object.keys(originalEnv).reduce((memo, key) => {
      memo[key] = `[[SCRUBBED IN CI]]`;
      return memo;
    }, {});
  process.env = sanitizedEnv;
}

We may want to allow a safelist of known safe keys that may need their actual value to be used and would be safe to print in the test.

Not saying this PR should block on an implementation like this but I'm curious of any ways we could solve the problem in as many places as possible.

@mrmicahcooper mrmicahcooper force-pushed the mc/hide-env-in-tests branch 3 times, most recently from 94f61bf to 8f8063e Compare April 27, 2020 20:43
@mrmicahcooper mrmicahcooper merged commit 157099c into master May 5, 2020
@mrmicahcooper mrmicahcooper deleted the mc/hide-env-in-tests branch May 5, 2020 13:22
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.

None yet

3 participants