Skip to content
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

mrc-5967 update e2e tests #229

Merged
merged 4 commits into from
Nov 13, 2024
Merged

mrc-5967 update e2e tests #229

merged 4 commits into from
Nov 13, 2024

Conversation

M-Kusumgar
Copy link
Collaborator

@M-Kusumgar M-Kusumgar commented Nov 6, 2024

just updating the e2e tests here, its passing but yh 12 mins feels unacceptable considering how simple wodin is, might do a bit of work to at least get it to less than 5 mins, main changes include:

  • removed .sort it was causing errors because sort modifies an array in place which means it mutates the array and the options displayed in dropdown are the wrong order for the e2e tests
  • monaco glyph implementation has changed, before the glyphs were with the line numbers so we could just go down the line numbers in a for loop and check the glyph, however now they are in a completely different overlay and are translated to match the line number so i just check the top css prop of the glyph
  • we had quite a fragile way to checking tooltip text has appeared on the page by finding the exact tooltip class, this has changed but i just expect that text to be visible on the page, i think that is a sufficient test that isnt as fragile

Comment on lines +76 to +82
// const hoverElem = `.${isGlyph ? "margin-" : ""}view-overlays div:nth-child(${line}) >> div`;
// const tooltipElem = `${isGlyph ? ".overlayWidgets" : ".overflowingContentWidgets"} .hover-contents div p`;
await page.hover(hoverElem, { force: true });
const tooltip = await page.locator(tooltipElem);
await tooltip.waitFor({ timeout: 1000 });
await expect(tooltip).toHaveText(message);
await expect(tooltip).toHaveCSS("visibility", "visible");
// const tooltip = await page.locator(tooltipElem);
// await tooltip.waitFor({ timeout: 2000 });
// await expect(tooltip).toHaveText(message);
// await expect(tooltip).toHaveCSS("visibility", "visible");
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ignore the commented out code and the block below, i remove them in the next PR, i accidentally pushed the cleaning out commented code commit to the next pr

@M-Kusumgar M-Kusumgar changed the title update e2e tests mrc-5967 update e2e tests Nov 6, 2024
Copy link
Contributor

@EmmaLRussell EmmaLRussell left a comment

Choose a reason for hiding this comment

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

Looks good!

GH is flagging a bunch of unused imports in these files too,

Comment on lines +36 to +41
let maxVariableLength = 0;
const variables = graphsGetters[GraphsGetter.allSelectedVariables];
for (let i = 0; i < variables.length; i++) {
maxVariableLength = Math.max(maxVariableLength, variables[i].length);
}
return maxVariableLength * LEGEND_WIDTH_PER_CHAR + LEGEND_LINE_PADDING;
Copy link
Contributor

Choose a reason for hiding this comment

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

This the change where the sort was reordering the result of the getter - because the getter result is cached and reused when some drop-down uses it? Which drop down was that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep exactly its being reused but sort also mutated the array in place, it was setting the wrong order in the select in LinkData.vue

Comment on lines 50 to 68
const expectMonacoDecoration = async (state: EditorStates, line: number, numOfLines: number, page: Page) => {
for (let i = 0; i < numOfLines; i += 1) {
const lineElement = await page.locator(`.view-overlays div:nth-child(${i + 1}) >> div`);
const glyphElement = await page.locator(`.margin-view-overlays div:nth-child(${i + 1}) >> div`);
if (i === line - 1) {
expect(lineElement).toHaveClass(`cdr editor-${state}-background`);
expect(glyphElement.nth(0)).toHaveClass(`cgmr codicon ${editorGlyphs[state]} ${state}-glyph-style ms-1`);
expect(glyphElement.nth(1)).toHaveClass("line-numbers lh-odd");
await expect(lineElement).toHaveClass(`cdr editor-${state}-background`);
} else if (i === numOfLines - 1) {
expect(lineElement).toHaveClass("current-line");
expect(glyphElement.nth(0)).toHaveClass("current-line current-line-margin-both");
expect(glyphElement.nth(1)).toHaveClass(/\bactive-line-number\b/);
expect(glyphElement.nth(1)).toHaveClass(/\bline-numbers\b/);
expect(glyphElement.nth(1)).toHaveClass(/\blh-odd\b/);
await expect(lineElement).toHaveClass("current-line");
} else {
expect(lineElement).toHaveCount(0);
expect(glyphElement).toHaveClass("line-numbers lh-odd");
await expect(lineElement).toHaveCount(0);
}
}

const lineHeight = await page.locator(".margin-view-overlays").evaluate(el => {
return window.getComputedStyle(el).getPropertyValue("line-height");
});
const glyphTop = await page.locator(`.${editorGlyphs[state]}`).evaluate(el => {
return window.getComputedStyle(el).getPropertyValue("top");
});
expect(glyphTop).toBe(`${(line - 1) * parseInt(lineHeight)}px`);
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this method could use some comments because I'm honestly quite confused about what's being checked for here! In the loop we're checking that there's only current-line/background styling for numOfLinesth line and its predecessor? And then the last part is checking that the glyph is on the specified line (by top)..?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep will add some comments but yh the for loop checks for the background colour of the line so the highlighting and this part is checking if the glyph is vertically aligned with the correct line number

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Comment on lines +449 to +453
// await expect((await page.innerText(":nth-match(.tooltip-inner, 2)")).trim()).toBe(
// "Set 10 (or any Set [number] combination) is reserved for default set names. " +
// "Please choose another set name or name this set back to its original name of 'Set 1'"
// );
// await expect(await page.isVisible(".param-name-input")).toBe(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be removed now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep removed in the linting PR (next one)

Comment on lines +60 to +63
test.use({
permissions: ["clipboard-read", "clipboard-write"]
})

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep it does! we have copy to clipboard buttons when we copy the code or link for a session and we check those like line 134 in this file

@M-Kusumgar
Copy link
Collaborator Author

GH is flagging a bunch of unused imports in these files too,

these will be solved in the next pr! (linting so seemed appropriate to wait until then)

@M-Kusumgar M-Kusumgar changed the base branch from mrc-5961 to mrc-5945 November 13, 2024 20:32
@M-Kusumgar M-Kusumgar merged commit ddfa50e into mrc-5945 Nov 13, 2024
1 check failed
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.

2 participants