Fix timeline feature error when base url is set #402

Meriem-BenIsmail · 2024-11-18T12:14:41Z

This PR fixes #393

github-actions · 2024-11-18T12:14:58Z

👈 Launch a Binder on branch Meriem-BenIsmail/jupyter-collaboration/baseurl-bug

davidbrochart · 2024-11-18T12:36:47Z

Failures seem to be unrelated (see #403).
I will look at that.

davidbrochart

Since #393 seems to suggest an issue with the server base URL, should you make use of it in this PR? The baseUrl is available in JupyterLab's page config.

Meriem-BenIsmail · 2024-11-19T09:36:30Z

Since #393 seems to suggest an issue with the server base URL, should you make use of it in this PR? The baseUrl is available in JupyterLab's page config.

The problem isn't actually with the baseUrl, it's just that the document name was being incorrectly extracted.

davidbrochart · 2024-11-19T09:42:28Z

Could you write a test that shows that this was indeed the cause of problem? In particular, I'd like to see that your fix works even if "api/collaboration/timeline" is present in the base URL.

Meriem-BenIsmail · 2024-11-20T08:33:51Z

Currently, the updated logic ensures that only the last occurrence of api/collaboration/timeline is picked up, so it should handle the base URL scenario as expected. Given this, the fix seems okay for the mentioned issue.

Would you still like me to add a specific test to verify this behavior?

I also want to mention that there’s another open PR (#401) that creates a test file for the timeline slider. If I add a test here, it would create or modify the same file, potentially leading to conflicts later. Let me know how you'd like to proceed—whether to hold off on tests for this PR until #401 is merged or go ahead and add them now.

davidbrochart · 2024-11-20T09:20:57Z

packages/docprovider/src/component.tsx

@@ -20,6 +20,7 @@ type Props = {
  contentType: string;
  format: string;
 };
+const DOCUMENT_TIMELINE_URL = 'api/collaboration/timeline';


We already have it here and here. Should it be in one place?

Yes I will fix that. Thanks for the feedback !

davidbrochart · 2024-11-20T09:22:56Z

Yes it would be nice to have a test. I merged #401.

davidbrochart · 2024-11-25T14:07:09Z

ui-tests/package.json

@@ -5,7 +5,9 @@
  "private": true,
  "scripts": {
    "start": "jupyter lab --config jupyter_server_test_config.py",
-    "test": "npx playwright test",
+    "start:timeline": "jupyter lab --config jupyter_server_test_config_timeline.py",


You can get rid of ui-tests/jupyter_server_test_config_timeline.py and do:

Suggested change

"start:timeline": "jupyter lab --config jupyter_server_test_config_timeline.py",

"start:timeline": "jupyter lab --ServerApp.base_url=/api/collaboration/timeline/",

davidbrochart · 2024-11-25T15:40:33Z

ui-tests/tests/timeline-slider.spec.ts

+
+  isTimeline && test.use({ autoGoto: false })
+
+  test('should fail if there are console errors when opening from path', async ({ page, tmpPath }) => {


I don't understand what this test does. It seems that you do nothing if isTimeline?

There is no need to execute this test in the added timeline test environment (with the set baseurl)
but this is the test for this PR that was merged #401

davidbrochart · 2024-11-25T15:40:55Z

ui-tests/tests/timeline-slider.spec.ts

    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 }) => {


Same comment as above.

davidbrochart · 2024-11-25T16:13:24Z

ui-tests/tests/timeline-slider.spec.ts

    await page.notebook.close();

+    if(!isTimeline)


Suggested change

if(!isTimeline)

davidbrochart · 2024-11-25T16:16:59Z

ui-tests/tests/timeline-slider.spec.ts

+
+test.describe('Timeline Slider', () => {
+
+  isTimeline && test.use({ autoGoto: false })


What is it for?

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 this test.use({ autoGoto: false })

And the part isTimeline && ?

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 (when isTimeline is true like in here TIMELINE_FEATURE=1 jlpm test:timeline )

Could you explain what you mean by "new environment" and "default environment"?

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 standard baseUrl (/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. If isTimeline is true, we disable the automatic navigation (test.use({ autoGoto: false })) that Playwright performs, as this navigation doesn’t respect the custom baseUrl and defaults to http://localhost:8888. Disabling it allows us to manually control the navigation to the appropriate custom environment.

Thanks. I think this is clearer:

Suggested change

isTimeline && test.use({ autoGoto: false })

if (isTimeline) {

test.use({ autoGoto: false });

}

davidbrochart

Thanks @Meriem-BenIsmail.

krassowski added the bug Something isn't working label Nov 18, 2024

Meriem-BenIsmail force-pushed the baseurl-bug branch from c17f130 to 87dc382 Compare November 19, 2024 08:17

davidbrochart reviewed Nov 19, 2024

View reviewed changes

davidbrochart reviewed Nov 20, 2024

View reviewed changes

Meriem-BenIsmail force-pushed the baseurl-bug branch from c40bd3a to a0b14fb Compare November 25, 2024 10:21

Meriem-BenIsmail added 8 commits November 25, 2024 14:23

fixed error with base url

fc73f07

modified filename extraction method.

504edf2

ui test timeline

6b9045a

ui test config done.

437f1e7

ui test for baseurl issue

0a96d4d

ui test for baseurl

65c4f28

pre-commit

6fe4174

test info

dc4f8ff

Meriem-BenIsmail force-pushed the baseurl-bug branch from e49ef76 to dc4f8ff Compare November 25, 2024 13:41

davidbrochart reviewed Nov 25, 2024

View reviewed changes

modified script for ui tests

c8296cc

davidbrochart reviewed Nov 25, 2024

View reviewed changes

Meriem-BenIsmail added 2 commits November 25, 2024 18:24

removed unused cnd

63348fe

clarify condition

42072ca

davidbrochart approved these changes Nov 26, 2024

View reviewed changes

davidbrochart merged commit 9802fd3 into jupyterlab:main Nov 26, 2024
27 checks passed

Meriem-BenIsmail deleted the baseurl-bug branch December 5, 2024 10:00

Meriem-BenIsmail restored the baseurl-bug branch January 6, 2025 13:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix timeline feature error when base url is set #402

Fix timeline feature error when base url is set #402

Meriem-BenIsmail commented Nov 18, 2024

github-actions bot commented Nov 18, 2024

davidbrochart commented Nov 18, 2024

davidbrochart left a comment

Meriem-BenIsmail commented Nov 19, 2024

davidbrochart commented Nov 19, 2024

Meriem-BenIsmail commented Nov 20, 2024

davidbrochart Nov 20, 2024

Meriem-BenIsmail Nov 20, 2024

davidbrochart commented Nov 20, 2024

davidbrochart Nov 25, 2024

davidbrochart Nov 25, 2024

Meriem-BenIsmail Nov 25, 2024

davidbrochart Nov 25, 2024

davidbrochart Nov 25, 2024

davidbrochart Nov 25, 2024

Meriem-BenIsmail Nov 25, 2024

davidbrochart Nov 25, 2024

Meriem-BenIsmail Nov 25, 2024

davidbrochart Nov 26, 2024

Meriem-BenIsmail Nov 26, 2024 •

edited

Loading

davidbrochart Nov 26, 2024

davidbrochart left a comment •

edited

Loading

	"start:timeline": "jupyter lab --config jupyter_server_test_config_timeline.py",
	"start:timeline": "jupyter lab --ServerApp.base_url=/api/collaboration/timeline/",


		isTimeline && test.use({ autoGoto: false })

		test('should fail if there are console errors when opening from path', async ({ page, tmpPath }) => {


		test.describe('Timeline Slider', () => {

		isTimeline && test.use({ autoGoto: false })

-  isTimeline && test.use({ autoGoto: false })
+  if (isTimeline) {
+    test.use({ autoGoto: false });
+  }

Fix timeline feature error when base url is set #402

Fix timeline feature error when base url is set #402

Conversation

Meriem-BenIsmail commented Nov 18, 2024

github-actions bot commented Nov 18, 2024

davidbrochart commented Nov 18, 2024

davidbrochart left a comment

Choose a reason for hiding this comment

Meriem-BenIsmail commented Nov 19, 2024

davidbrochart commented Nov 19, 2024

Meriem-BenIsmail commented Nov 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidbrochart commented Nov 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Meriem-BenIsmail Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidbrochart left a comment • edited Loading

Choose a reason for hiding this comment

Meriem-BenIsmail Nov 26, 2024 •

edited

Loading

davidbrochart left a comment •

edited

Loading