pam/integration-tests: Avoid downloading chromium on each test run

[adombeck]

The $HOME environment variable wasn't set in the environment which the vhs command runs in, resulting in chromium being downloaded to .cache/rod in the current working directory, which is a temporary directory. In effect, chromium was being downloaded each time a test using vhs was executed, which made the test take ~1m longer.

UDENG-7819

[adombeck]

@3v1n0 do you see any reason why we shouldn't just pass the whole os.Environ to the vhs command here?

[3v1n0]

I would prefer to control what VHS sees and ideally I'd prefer not to be potentially interfered by the developers HOME, and so to shared here. So wondering, can we just set the XDG_CACHE_HOME to make rod use it?

[3v1n0]

BTW likely this can be confined to the cases in which: we're in CI, and no chromium or google-chrome[-unstable] is in PATH, no?

[3v1n0]

Another option is to just set HOME for vhs itself, but then set it to some test path for the tests themselves

[adombeck]

The directory used by rod is: filepath.Join(os.Getenv("HOME"), ".cache", "rod", "browser")

https://github.com/go-rod/rod/blob/393ac0d60b53f3c4a9b2a6504d250cbada55b546/lib/launcher/browser.go#L73

So we could set $HOME to some temporary directory, but that could have unexpected side effects. IMO we should just keep the environment variables from the go test environment. Or is vhs more prone to interference by the developer's home directory than everything else we do in the tests (where we don't unset the environment)?

[adombeck]

Ah, I see. In the SSH tests we already set HOME to a temporary directory in tapeData.Env: https://github.com/ubuntu/authd/blob/0609f97a414c0b9fe366f4342e91a89d6bcf347c/pam/integration-tests/ssh_test.go#L464-L464

That means that the commands executed by the tape won't see the HOME variable we're setting for vhs anyway, right? Should I add a commit which always sets td.Env["HOME"] = t.TempDir() in RunVhs?

[adombeck]

Ok, I've fixed it there in go-rod/rod#1213

I added a TODO comment to set XDG_CACHE_HOME instead of HOME once that PR was merged

[3v1n0]

That means that the commands executed by the tape won't see the HOME variable we're setting for vhs anyway, right?

Yeah, that is fine, my point was: maybe we can just do it for all the tests now (also CLI and Native) and drop it the specific case in ssh tests?

[adombeck]

my point was: maybe we can just do it for all the tests now (also CLI and Native) and drop it the specific case in ssh tests?

that's what I meant with "Should I add a commit which always sets td.Env["HOME"] = t.TempDir() in RunVhs"

[adombeck]

done

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.10%. Comparing base (b3a6cc9) to head (5ee79e3).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1040      +/-   ##
==========================================
- Coverage   88.11%   88.10%   -0.02%     
==========================================
  Files          85       85              
  Lines        6026     6026              
  Branches      111      111              
==========================================
- Hits         5310     5309       -1     
- Misses        660      661       +1     
  Partials       56       56              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[adombeck]

@3v1n0 anything missing here?