Skip to content

Fix tests and lock Yarn version#741

Open
mnvsngv wants to merge 9 commits into
looker-open-source:masterfrom
mnvsngv:feat/fix-test-infra
Open

Fix tests and lock Yarn version#741
mnvsngv wants to merge 9 commits into
looker-open-source:masterfrom
mnvsngv:feat/fix-test-infra

Conversation

@mnvsngv
Copy link
Copy Markdown

@mnvsngv mnvsngv commented May 6, 2026

This PR is preparing for a subsequent PR to fix an OAuth vulnerability. I'm fixing up the test infrastructure first so that I can include new test cases in the next PR.

@mnvsngv mnvsngv requested a review from a team as a code owner May 6, 2026 20:36
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request updates the project configuration by specifying a package manager and upgrading the chai-http dependency to version 4.3.0. It also enables server-side testing by uncommenting relevant imports and setup code, and adds a cleanup step to restore sinon state after each test. Feedback includes a recommendation to update the corresponding @types/chai-http package for type safety and a suggestion to reorder imports in the test files to follow standard practices.

Comment thread package.json
Comment thread test/test_server.ts Outdated
Comment on lines +7 to +11

const chaiHttp = require("chai-http")
chai.use(chaiHttp)

import "../src/actions/index"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Import statements should be grouped together at the top of the file, before other code like variable declarations or function calls. This improves readability and follows standard TypeScript practices.

Suggested change
const chaiHttp = require("chai-http")
chai.use(chaiHttp)
import "../src/actions/index"
import "../src/actions/index"
const chaiHttp = require("chai-http")
chai.use(chaiHttp)

@mnvsngv mnvsngv force-pushed the feat/fix-test-infra branch from 75b7a66 to 24741be Compare May 6, 2026 21:58
Comment thread test/test.ts
import "./test_json_detail_stream"
import "./test_oauth_action"
// import "./test_server"
import "./test_server"
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I'm adding back the test_server tests so that I can add test cases for OAuth in my next PR.

Comment thread test/test.ts Outdated
import "../src/actions/jira/test_jira"
import "../src/actions/kloudio/test_kloudio"
import "../src/actions/marketo/test_marketo"
// import "../src/actions/marketo/test_marketo"
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Marketo isn't actively maintained and its tests are failing after I added the test_server. So I'm commenting those out.

Comment thread package.json
"node": "20.16.0",
"yarn": ">=1.19.1"
},
"packageManager": "yarn@1.19.1",
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This allows Corepack to pick up the correct version of Yarn to be used for local development.

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.

1 participant