-
-
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 8 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,21 @@ | ||
# Copyright (c) Jupyter Development Team. | ||
# Distributed under the terms of the Modified BSD License. | ||
|
||
"""Server configuration for integration tests. | ||
|
||
!! Never use this configuration in production because it | ||
opens the server to the world and provide access to JupyterLab | ||
JavaScript objects through the global window variable. | ||
""" | ||
from typing import Any | ||
|
||
from jupyterlab.galata import configure_jupyter_server | ||
|
||
c: Any | ||
configure_jupyter_server(c) # noqa | ||
|
||
c.ServerApp.base_url = "/api/collaboration/timeline/" | ||
|
||
|
||
# Uncomment to set server log level to debug level | ||
# c.ServerApp.log_level = "DEBUG" |
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 }) | ||||||||||
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. What is it for? 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. So I found that even with different configuration, somehow playwright overwrites the baseurl and goes to the default one ( 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. And the part 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. 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 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. 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 commentThe 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
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. Thanks. I think this is clearer:
Suggested change
|
||||||||||
|
||||||||||
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.
You can get rid of
ui-tests/jupyter_server_test_config_timeline.py
and do: