Skip to content

Stage-0: Verify tests and coverage #1387

Description

@l2ysho

Important

As we are going to refactor big chunk of auth flow, lets first quickly check what is test coverage of this part right now.
Lets check:

  • untested parts of auth (parts we are going to drop can be skipped)
  • false positive tests

We should ensure we are not going to refactor parts not tested at all. (At least some meaningful happy path is required). Let's not spend more time than needed here.

Safe to refactor?

Area Tests Verdict
credentials.ts — backend choice, file round-trip, ensureMigrated, clearKeyringSecrets 23
local-api-server.ts — CORS, bad/missing token, 404, bad JSON 9
login → auth.json → logout, file backend log_in_out.test.ts:31 deep-equals against live user('me').get()
proxy password → apify run child env (APIFY_ENV_VARS.PROXY_PASSWORD) run.test.ts:128, :169, :209 deep-equal against auth.proxy.password ✅ file backend only — the pin that protects moving the proxy password per-profile in Stage-1
getLoggedClient keyring branch — token write, proxy write, stripping secrets from auth.json none ⚠️
keyring write fails → downgradeBackendToFile none ⚠️
skipIfUnchanged (avoids Keychain prompt every command) fake, see F1 ⚠️
auth.json written 0o600 none anywhere ⚠️
Bundle keyring loader __APIFY_KEYRING_NATIVE_SUBPACKAGE__ none ⚠️ needs build smoke test
keyring logout, updateUserId(null) none ⚠️
getLocalUserInfo "stale credentials" throw (utils.ts:113) none ⚠️ Stage-1 rewrites exactly this decision
resolveToken — the chokepoint behind all 94 getLoggedClient* call sites none ⚠️ needs a happy-path pin. Its APIFY_TOKEN branch is still undecided (#1328 leans to dropping it, #1246 / #720 want it kept) — but the function is what Stage-2/3 rewire for --profile

Also: src/commands/auth/* is reachable in tests only through the top-level aliases — log_in_out.test.ts:20-21 imports src/commands/login.js / logout.js, useAuthSetup.ts:8 imports LoginCommand. Both run only under test:api / test:e2e, which need TEST_USER_TOKEN. pnpm test:local covers the auth commands 0%.

Why: one line

useAuthSetup.ts:45 sets APIFY_DISABLE_KEYRING=1. run-cli.ts:55 does it for e2e too. So the keyring — the default for real users — is never hit through a command. One fixture variant on the keyring backend closes 4 of the ⚠️ rows at once, and extends the run.ts proxy-password assertions to the keyring path.

6 tests that can't fail

Mutation-verified: deleted the branch, suite stayed green.

# Test Why
F1 credentials.test.ts:107 skipIfUnchanged no-op Compares auth.json bytes. A rewrite is byte-identical, so "skipped" and "wrote again" look the same. Deleting the whole block → 23/23 green. Needs a writeFileSync spy.
F2 credentials.test.ts:115 Green with the option removed.
F3 credentials.test.ts:166 marker re-entry guard File-backend re-migration is idempotent. marker === 'keyring', where the guard matters, has no test.
F4 credentials.test.ts:258 JSON.parse of the fixture it just wrote. Asserts itself.
F5 e2e/auth/login.test.ts:26 prints token stdout.length > 0 — printing anything passes.
F6 e2e/auth/login.test.ts:18 + log_in_out.test.ts:24 bad token One stderr substring, pass offline, no exit-code check — and login exits 0 on a bad token today (#1254), so the right assertion would fail.

No it.skip, no expect(true), no zero-assertion tests. The problem isn't fake assertions — it's assertions that can't tell the branch apart.

Stage-0 exit criteria

  • Keyring-backend fixture variant — closes 4 gaps, highest value
  • writeFileSync spy for F1/F2
  • expect(statSync(AUTH_FILE_PATH()).mode & 0o777).toBe(0o600) — one line
  • Point the existing keyringFailures fake (credentials.test.ts:32) at setToken/setProxyPassword, not just ensureMigrated
  • F5: compare auth token stdout to the stored token
  • Logout clears state on both backends — no token in the keyring, none in auth.json, file removed. This is the safety net for re-keying secrets in Stage-1, so assert state, not the exit code
  • resolveToken happy path — returns the stored token when nothing overrides it. ~5 lines, test:local, independent of how the APIFY_TOKEN question lands
  • getLocalUserInfo throws on stale credentials without user metadata (utils.ts:113)

Skip: login/logout alias commands, more ensureMigrated cases, Stryker, pinning the APIFY_TOKEN branch of resolveToken. secrets.ts is Actor env vars, not credentials — don't count it.

Coverage tooling optional: 1 dep, ~10 min. All 6 fakes sit on executed lines, so it'd report credentials.ts near 100%. e2e coverage isn't feasible (subprocess against dist/).

Bugs found — both already tracked, filing nothing

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    t-buildersIssues owned by the Builders team.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions