-
-
Notifications
You must be signed in to change notification settings - Fork 32
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 timeline feature error when base url is set #402
Changes from 9 commits
fc73f07
504edf2
6b9045a
437f1e7
0a96d4d
65c4f28
6fe4174
dc4f8ff
c8296cc
63348fe
42072ca
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 |
---|---|---|
@@ -0,0 +1,33 @@ | ||
/* | ||
* Copyright (c) Jupyter Development Team. | ||
* Distributed under the terms of the Modified BSD License. | ||
*/ | ||
|
||
/** | ||
* Configuration for Playwright using default from @jupyterlab/galata | ||
*/ | ||
const baseConfig = require('@jupyterlab/galata/lib/playwright-config'); | ||
|
||
module.exports = { | ||
...baseConfig, | ||
workers: 1, | ||
webServer: { | ||
command: 'jlpm start:timeline', | ||
url: 'http://localhost:8888/api/collaboration/timeline/lab', | ||
timeout: 120 * 1000, | ||
reuseExistingServer: !process.env.CI | ||
}, | ||
expect: { | ||
toMatchSnapshot: { | ||
maxDiffPixelRatio: 0.01 | ||
} | ||
}, | ||
projects: [ | ||
{ | ||
name: 'timeline-tests', | ||
testMatch: 'tests/**/timeline-*.spec.ts', | ||
testIgnore: '**/.ipynb_checkpoints/**', | ||
timeout: 120 * 1000 | ||
} | ||
] | ||
}; |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -23,17 +23,51 @@ async function openNotebook(page: Page, notebookPath: string) { | |||
await page.waitForSelector('.jp-Notebook', { state: 'visible' }); | ||||
} | ||||
|
||||
test.describe('Open from Path', () => { | ||||
|
||||
test('should fail if there are console errors', async ({ page, tmpPath }) => { | ||||
const isTimelineEnv = process.env.TIMELINE_FEATURE || "0"; | ||||
const isTimeline = parseInt(isTimelineEnv) | ||||
|
||||
test.describe('Timeline Slider', () => { | ||||
|
||||
isTimeline && test.use({ autoGoto: false }) | ||||
|
||||
test('should fail if there are console errors when opening from path', async ({ page, tmpPath }) => { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand what this test does. It seems that you do nothing if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no need to execute this test in the added timeline test environment (with the set baseurl) |
||||
if (isTimeline) { | ||||
console.log('Skipping this test.'); | ||||
return; | ||||
} | ||||
const pageErrors = await capturePageErrors(page); | ||||
|
||||
await page.notebook.createNew(); | ||||
await page.notebook.save(); | ||||
await page.notebook.close(); | ||||
|
||||
if(!isTimeline) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||
await openNotebook(page, `${tmpPath}/Untitled.ipynb`); | ||||
|
||||
expect(pageErrors).toHaveLength(0); | ||||
}); | ||||
|
||||
test('should display in status bar without console errors when baseUrl is set', async ({ page, baseURL }) => { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment as above. |
||||
|
||||
if (!isTimeline) { | ||||
console.log('Skipping this test.'); | ||||
return; | ||||
} | ||||
|
||||
await page.goto('http://localhost:8888/api/collaboration/timeline') | ||||
|
||||
const pageErrors = await capturePageErrors(page); | ||||
|
||||
await page.notebook.createNew(); | ||||
|
||||
const historyIcon = page.locator('.jp-mod-highlighted[title="Document Timeline"]'); | ||||
await expect(historyIcon).toBeVisible(); | ||||
|
||||
await historyIcon.click(); | ||||
|
||||
const slider = page.locator('.jp-timestampDisplay'); | ||||
await expect(slider).toBeVisible(); | ||||
|
||||
expect(pageErrors).toHaveLength(0); | ||||
}); | ||||
}); |
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.
What is it for?
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.
So I found that even with different configuration, somehow playwright overwrites the baseurl and goes to the default one (
http://localhost:8888
) by default so we need to use thistest.use({ autoGoto: false })
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.
And the part
isTimeline &&
?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.
isTimeline is the environment variable used to determine whether we are testing with the new environment or the default one. So we need to do this
test.use({ autoGoto: false })
only if we are testing with the new env (whenisTimeline
is true like in hereTIMELINE_FEATURE=1 jlpm test:timeline
)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.
Could you explain what you mean by "new environment" and "default environment"?
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 "new environment" refers to the testing setup needed for this PR, which uses a custom
baseUrl
(/api/collaboration/timeline/
) as opposed to the "default environment," where tests run with the standardbaseUrl
(/lab
or/
). The new environment is specifically configured to validate the Timeline functionality in isolation, whereas the default environment is used for general tests without the Timeline-specific context.isTimeline
is used to identify which environment is active. IfisTimeline
is true, we disable the automatic navigation (test.use({ autoGoto: false })
) that Playwright performs, as this navigation doesn’t respect the custombaseUrl
and defaults tohttp://localhost:8888
. Disabling it allows us to manually control the navigation to the appropriate custom environment.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.
Thanks. I think this is clearer: