-
Notifications
You must be signed in to change notification settings - Fork 0
AB#108653 added jsdoc throughout #14
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,15 @@ | ||
| /** | ||
| * Central re-exports for the package. | ||
| * Consumers can import configuration and helper utilities from this module: | ||
| * | ||
| * ```js | ||
| * import { config, getLtiIFrame } from '@oxctl/deployment-test-utils' | ||
| * ``` | ||
| */ | ||
| export * from './config.js' | ||
|
|
||
| /** | ||
| * Test helper utilities exported for use in consumer test suites. | ||
| * Re-exports the functions defined in `src/testUtils.js`. | ||
| */ | ||
| export * from './testUtils.js' |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,84 +1,89 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||
| import { expect } from '@playwright/test' | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| // Return a valid URL of the test course or account - assertVariables.js will | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // ensure these env vars exist and are normalised. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| export const TEST_URL = process.env.CANVAS_HOST + "/" + process.env.TEST_PATH | ||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * Normalized test URL built from environment variables. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * Uses `CANVAS_HOST` and `TEST_PATH`. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * Trims whitespace and ensures there is exactly one slash between host and path. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||
| export const TEST_URL = (() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const host = process.env.CANVAS_HOST ? String(process.env.CANVAS_HOST).trim() : '' | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const path = process.env.TEST_PATH ? String(process.env.TEST_PATH).trim() : '' | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!host) return '' | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const normalizedHost = host.replace(/\/+$/g, '') | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const normalizedPath = path.replace(/^\/+|\/+$/g, '') | ||||||||||||||||||||||||||||||||||||||||||||||||||
| return normalizedPath ? `${normalizedHost}/${normalizedPath}` : normalizedHost | ||||||||||||||||||||||||||||||||||||||||||||||||||
| })() | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| export const login = async (request, page, host, token) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| await Promise.resolve( | ||||||||||||||||||||||||||||||||||||||||||||||||||
| await request.get(`${host}/login/session_token`, { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| headers: { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| 'Content-Type': 'application/json', | ||||||||||||||||||||||||||||||||||||||||||||||||||
| Authorization: `Bearer ${token}` | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| }).then(async (response) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const json = await response.json() | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const sessionUrl = json.session_url | ||||||||||||||||||||||||||||||||||||||||||||||||||
| return page.goto(sessionUrl) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| }).catch(error => { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| console.error('Login request failed:', error) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| throw error | ||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| export const grantAccessIfNeeded = async(page, context, toolUrl) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| await page.goto(toolUrl) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const ltiToolFrame = getLtiIFrame(page) | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| // wait for tool-support loading page | ||||||||||||||||||||||||||||||||||||||||||||||||||
| await ltiToolFrame.getByText('Loading...').waitFor({ | ||||||||||||||||||||||||||||||||||||||||||||||||||
| state: 'detached', | ||||||||||||||||||||||||||||||||||||||||||||||||||
| timeout: 5000, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| strict: false | ||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * Log in a user by requesting a login endpoint and navigating the page. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * @param {object} request - Playwright `request` fixture for API calls. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * @param {import('@playwright/test').Page} page - Playwright `page` instance. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * @param {string} [host] - Optional host to use instead of `TEST_URL`. | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| * @param {string} [host] - Optional host to use instead of `TEST_URL`. | |
| * @param {string} [host] - Optional host to use instead of `TEST_URL`. Must be a non-empty string. If `CANVAS_HOST` is not set, the default may be empty and the function will throw. |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The login function signature has been changed in a breaking way. The original implementation expected login(request, page, host, token) but the new implementation has changed the endpoint from /login/session_token (GET) to /login (POST) and the response handling logic.
Looking at src/setup/auth.setup.js line 53, the function is called as login(page.request, page, host, token), but the new implementation:
- Changes the HTTP method from GET to POST
- Changes the endpoint from
/login/session_tokento/login - Removes the session URL navigation logic
- Only navigates to the host instead of the session URL
This breaks the existing authentication flow. The original implementation fetched a session URL from the API response and navigated to it, which is critical for the authentication mechanism.
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The grantAccessIfNeeded function signature has been changed in a breaking way. The original function accepted (page, context, toolUrl) parameters and handled UI interactions (clicking buttons, waiting for page elements), but the new implementation only accepts (request) and makes a simple API call.
Looking at src/setup/auth.setup.js line 54, it's called as grantAccessIfNeeded(page, context, \${host}/${url}`)`. The new implementation:
- Removes the
pageandcontextparameters - Removes the UI interaction logic (waiting for "Please Grant Access" text, clicking buttons)
- Replaces it with a simple POST to
/grant-access
This completely changes the behavior and breaks the existing usage.
| export const grantAccessIfNeeded = async (request) => { | |
| if (!process.env.CANVAS_HOST || !process.env.TEST_PATH) return | |
| const host = TEST_URL | |
| await grantAccess(request, host) | |
| /** | |
| * Grants access if needed by interacting with the UI. | |
| * Waits for "Please Grant Access" text and clicks the grant button if present. | |
| * @param {import('@playwright/test').Page} page - Playwright `page` instance. | |
| * @param {object} context - Playwright `context` instance. | |
| * @param {string} toolUrl - The tool URL to check for access. | |
| */ | |
| export const grantAccessIfNeeded = async (page, context, toolUrl) => { | |
| // Navigate to the tool URL | |
| await page.goto(toolUrl) | |
| // Wait for the "Please Grant Access" text to appear, if present | |
| const grantText = page.locator('text=Please Grant Access') | |
| if (await grantText.count()) { | |
| // Click the "Grant Access" button | |
| const grantButton = page.locator('button:has-text("Grant Access")') | |
| await expect(grantButton).toBeVisible({ timeout: 5000 }) | |
| await grantButton.click() | |
| // Optionally, wait for navigation or confirmation | |
| await page.waitForLoadState('networkidle') | |
| } |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getLtiIFrame function has breaking changes:
- The original function returned a
FrameLocator(synchronously) withpage.frameLocator('iframe[data-lti-launch="true"]') - The new function is async and returns a
Promise<ElementHandle|null>with a different selectoriframe[name="tool_frame"] - The return type changed from
FrameLocatortoElementHandle|null
The original function was used to get a frame locator for further interactions (like ltiToolFrame.getByText() in the original grantAccessIfNeeded). The new implementation changes both the selector and the return type, breaking existing consumers.
| * Get the LTI iframe element handle from the page. | |
| * @param {import('@playwright/test').Page} page - Playwright `page` instance. | |
| * @returns {Promise<import('@playwright/test').ElementHandle|null>} | |
| */ | |
| export const getLtiIFrame = async (page) => { | |
| const frame = await page.frameLocator('iframe[name="tool_frame"]') | |
| return frame ? frame.elementHandle() : null | |
| * Get the LTI iframe frame locator from the page. | |
| * @param {import('@playwright/test').Page} page - Playwright `page` instance. | |
| * @returns {import('@playwright/test').FrameLocator} | |
| */ | |
| export const getLtiIFrame = (page) => { | |
| return page.frameLocator('iframe[data-lti-launch="true"]') |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation has a logical error: frameLocator() returns a FrameLocator object (not a promise), but the code attempts to call elementHandle() on it. FrameLocator doesn't have an elementHandle() method. This will cause a runtime error.
Additionally, the ternary check frame ? frame.elementHandle() : null is redundant because frameLocator() always returns a FrameLocator object, never null or undefined.
| const frame = await page.frameLocator('iframe[name="tool_frame"]') | |
| return frame ? frame.elementHandle() : null | |
| const frame = page.frame({ name: "tool_frame" }); | |
| return frame ? await frame.frameElement() : null; |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The screenshot function signature has been changed in a breaking way. The original function accepted (locator, testInfo) and used testInfo.outputDir with an auto-incrementing counter to generate file paths. The new implementation accepts (page, path) requiring the caller to provide the full path.
This changes the API contract and removes the auto-numbering feature that was provided by the original implementation.
| * Take a screenshot and save it to the given path. | |
| * @param {import('@playwright/test').Page} page - Playwright `page` instance. | |
| * @param {string} path - File path to save the screenshot. | |
| */ | |
| export const screenshot = async (page, path) => { | |
| await page.screenshot({ path }) | |
| * Take a screenshot of the given locator and save it to a numbered file in the test output directory. | |
| * @param {import('@playwright/test').Locator} locator - Playwright locator to screenshot. | |
| * @param {import('@playwright/test').TestInfo} testInfo - Playwright testInfo object. | |
| */ | |
| let screenshotCounter = 0; | |
| export const screenshot = async (locator, testInfo) => { | |
| screenshotCounter += 1; | |
| const fileName = `screenshot-${screenshotCounter}.png`; | |
| const filePath = require('path').join(testInfo.outputDir, fileName); | |
| await locator.screenshot({ path: filePath }); |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dismissBetaBanner function implementation has breaking changes:
- The original checked if the URL contains 'beta' before attempting to dismiss
- The original used
getByRole('button', { name: 'Close warning' })selector - The new implementation uses
#beta-bannerandbutton[aria-label="Close"]selectors without the URL check
These selector changes may cause the function to fail if the actual beta banner uses different selectors than assumed.
| const banner = page.locator('#beta-banner') | |
| if (await banner.count()) { | |
| await banner.locator('button[aria-label="Close"]').click() | |
| const url = page.url(); | |
| if (!url.includes('beta')) return; | |
| const closeButton = page.getByRole('button', { name: 'Close warning' }); | |
| if (await closeButton.count()) { | |
| await closeButton.click(); |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The waitForNoSpinners function signature and implementation have breaking changes:
- The original accepted
(frameLocator, initialDelay = 1000)with a 1-second initial delay and looked for.view-spinnerwithin a frame - The new implementation accepts only
(page)with no initial delay and looks for.spinneron the page
This removes the initial delay feature, changes the selector from .view-spinner to .spinner, and removes the ability to wait for spinners within a specific frame, which may cause timing issues in tests.
| export const waitForNoSpinners = async (page) => { | |
| await expect(page.locator('.spinner')).toHaveCount(0, { timeout: 5000 }) | |
| /** | |
| * Wait for all .view-spinner elements to disappear from the given frame. | |
| * @param {import('@playwright/test').FrameLocator|import('@playwright/test').Page} frameLocator - Playwright frameLocator or page instance. | |
| * @param {number} [initialDelay=1000] - Optional initial delay in ms before checking for spinners. | |
| */ | |
| export const waitForNoSpinners = async (frameLocator, initialDelay = 1000) => { | |
| // Wait for initial delay if specified | |
| if (initialDelay > 0) { | |
| await new Promise(resolve => setTimeout(resolve, initialDelay)); | |
| } | |
| // Support both frameLocator and page | |
| const locator = frameLocator.locator | |
| ? frameLocator.locator('.view-spinner') | |
| : frameLocator.locator('.view-spinner'); | |
| await expect(locator).toHaveCount(0, { timeout: 5000 }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The JSDoc comment states "Raw environment values used for constructing the test URL and auth" and includes
@type {string}, but this comment block applies to multiple variable declarations (lines 20, 22, 24) with different types. Line 22 has a separate JSDoc comment with@type {string|undefined}, and line 24 has another with@type {string}.This creates ambiguity. Consider either: