18 translation version name in title bar #1979

b-w-cole · 2024-07-25T18:13:09Z

Description

A test to cover this test case: Sefaria/Sefaria-Playwright-Tests#18

Includes:

Parameterized tests for all Interface and Source language combinations
Abstracted source language function for this and future purposes
New Global variable for source language selection, pointing to DOM locators.

Code Changes

The following changes were made to the files below

Notes

Issue noted with Hebrew sources - the translation title doesn't appear by default. Such cases are commented out in the test parameters.

Sefaria/Sefaria-Playwright-Tests#18 TODO: Decide if we want to test for default translation title.

Issue noted for Hebrew sources - no default translation is selected.

b-w-cole · 2024-07-26T01:56:53Z

@saengel , this one isn't quite ready to merge. I'm wondering about your opinion on the file translation-version-name-appears-in-title.spec.ts, and the commented out tests. Do you want me to test for the existence of a default translation title? This will fail for Hebrew source language texts, as the default translation isn't selected in the translation side bar.

saengel · 2024-08-01T17:34:47Z

@saengel , this one isn't quite ready to merge. I'm wondering about your opinion on the file translation-version-name-appears-in-title.spec.ts, and the commented out tests. Do you want me to test for the existence of a default translation title? This will fail for Hebrew source language texts, as the default translation isn't selected in the translation side bar.

Hi @b-w-cole - thank you again! Another fantastic PR - much appreciated!

I think for Hebrew source language texts, let's keep them commented out for now. Let me know when it's ready for a review.

b-w-cole · 2024-08-09T21:32:59Z

Hi @saengel ! Sorry for the long wait. I tweaked the Hebrew source language tests to be their own separate test, as the logic needs to be slightly different.

However, I noticed a quirk with the Hebrew Interface / Hebrew Source translations.

Load Hebrew Interface and Source text
Navigate to Translations
Select 'חומש רש״י, רבי שרגא זילברשטיין'
'The Rashi chumash by Rabbi Shraga Silverstein' (in English) will appear in the translation title.

I would expect the Hebrew translation title to appear in the translation title.

Also the source text switches itself to Bilingual.

I will switch this test parameter to Shocken's translation for now and recommit.

Also allowing default translation validations to be skipped for Hebrew language source texts

b-w-cole · 2024-08-09T23:28:48Z

Hi @saengel , I was able to consolidate the English and Hebrew tests with a little rearranging. Additionally, I added bilingual support.

Issue: Tests time out on assertions looking for text in the wrong language Solution: Clearing the cookies resets the cookie-setting process and allows language cookies to be refreshed for each context.

saengel

Hi @b-w-cole - overall looks really great! Some tricky cases you had to figure out here, and appreciate how you found a way... even when there were unexpected edge cases.

I left a few comments, mostly for us to discuss (curious to hear your thoughts as well). We can go from there. As always, great work!

saengel · 2024-10-07T06:58:50Z

e2e-tests/tests/interface-language-is-sticky.spec.ts

+        await page.getByRole('radiogroup', {name: 'Language'}).locator(`${sourceLanguageToggle}`).click()
+
+        // Locating the source text segment, then verifying translation
+        await expect(page.locator('div.segmentNumber').first().locator('..').locator('p')).toContainText(`${expectedSourceText}`)


We had discussed focusing on testing user-facing attributes (reference to the playwright docs here) versus locators, you had mentioned that you try to do that as much as possible - unless the case necessitates locators. I'm assuming this is a case you needed locators, do you mind elaborating on why? Will help me better understand how we're able to test our code.

(And, if this happens to be a case that can be rewritten to test user-facing attributes, that would be great as well!)

Ok I see. What I was doing here was using the "Pick Locator" tool in VSCode. Let me see if I can neaten it up.

I was able to condense this to await expect(page.getByRole('region').locator('.segmentText span').last()).toContainText("expectedText"), as well as remove some other locators, but I can only go so far, as I need to pinpoint exactly where the text is showing up. There's not really roles associated with the divs and spans for text, so I use the CSS locators instead. I could use getByText(), but if I do that, I cannot test.

Additionally, I was able to simplify the process for changing source language and remove those locators. **Edit: In the photo, I simplified further and now we're back to locators. :/ **

Hello! On this topic again, I took another look at this, and found that the best way is with the locator pointing to div classes. I tried to use sensible CSS classes where it is clearer what the locator is pointing to.

I am also submitting a correction to the commit I submitted a week ago. I was thinking about it at the time that

await expect(page.getByRole('link', {name: currentlySelectedText}).locator('..')).toContainText(defaultTranslation!)

would be appropriate for translation-version-name-appears-in-title.spec.ts, but I'd like to change it. While the test passes, it doesn't flow very well through the DOM. Also, it is not at all recommended by the course I'm taking, so there it is that.

Instead, I would like to change this line to be:

await expect(page.locator('div.version-with-preview-title-line').filter({ hasText: currentlySelectedText })).toContainText(translationName!)

It flows much more naturally in the DOM, looking through the document for all version title lines containing 'Currently Selected' and checking if this also contains the translation we want.

What do you think?

Hi Brandon, thank you for the thoughtful explanations and revisions here. I think this improvement is great. Again, we try to prefer user-facing attributes as much as possible - but sometimes we need to rely on locators and that should be fine. Thank you for these improvements.

saengel · 2024-10-07T06:59:17Z

e2e-tests/tests/interface-language-is-sticky.spec.ts

+
+        // Checking out the second part of the text, if 'Bilingual' is selected
+        if(`${sourceLanguage}` === 'Bilingual'){
+            await expect(page.locator('div.segmentNumber').first().locator('..').locator('p span').last()).toContainText(`${expectedBilingualText}`)


Same question here in terms of using a locator.

Thanks for keeping me on track with these. I'm continuing to learn, and you're helping me out. There's more I'd like to simplify in this for sure. I'll let you know.

saengel · 2024-10-07T07:00:41Z

e2e-tests/tests/translation-version-name-appears-in-title.spec.ts

+
+         // Navigate to the Translations sidebar by clicking on the text title
+         //Clicks on בראשית א׳ / Genesis I
+         await page.locator('h1').click()


Same comment throughout this file regarding locator use - I didn't comment in each location, just a general curiosity to see if it's possible to rewrite any of these.

Hmm... I can change it to await page.getByRole('heading').first().click(). This still works, and it is using best practices. What do you think?

I think that would be great, thank you for the suggestion!

saengel · 2024-10-07T07:01:41Z

e2e-tests/tests/translation-version-name-appears-in-title.spec.ts

+         await page.waitForSelector('h3')
+
+        // Check if the default translation in the title matches the selected translation
+        // NOTE: We are skipping checking for the default translation here, due to the Hebrew text being default Masoretic


Can we rewrite to say "due to the Hebrew text not displaying a version title". That might be a more accurate description of what's going on in terms of site functionality (even if the root reason behind it is what you described here, for Tanakh texts at least)

Sure thing! I will make this change.

Great, thank you!

saengel · 2024-10-07T07:02:43Z

e2e-tests/tests/translation-version-name-appears-in-title.spec.ts

+        // TODO: 4th translation, handling Hebrew Interface translations in Hebrew.  For example: 'חומש רש״י, רבי שרגא זילברשטיין' should appear in the translation title as written.
+        const translationNames = ['The Schocken Bible, Everett Fox, 1995 ©', '«Да» project']
+
+        // Utilizing the traditional for-loop as there are async issues with foreach


Helpful comment! Thank you!

saengel · 2024-10-07T07:03:06Z

e2e-tests/utils.ts

-        await changeLanguage(page, language);
-        langCookies = await context.cookies();
-    }
+    // If a cookie already has contents, clear it so that the language cookie can be reset


This is great, thank you again for the efforts to figure this out!

saengel · 2024-10-07T07:04:46Z

e2e-tests/tests/interface-language-is-sticky.spec.ts

+    // Hebrew Interface and Hebrew Source
+    {interfaceLanguage: 'Hebrew', interfaceLanguageToggle: LANGUAGES.HE, 
+        sourceLanguage: 'Hebrew', sourceLanguageToggle: SOURCE_LANGUAGES.HE, 
+        expectedSourceText: 'רֵאשִׁ֖ית בָּרָ֣א אֱלֹהִ֑ים אֵ֥ת הַשָּׁמַ֖יִם וְאֵ֥ת הָאָֽרֶץ׃', expectedBilingualText: '', expectedInterfaceText: 'מקורות' },


Not a super huge deal (especially since copy and paste with Hebrew text into most IDEs can be really finicky) but the first letter of the verse (the ב) is missing in every instance of Hebrew expectedSourceText. Is it possible to add it in for clarity? Just so it makes more sense to people reading the tests.

Oh shoot, sorry about that! It is fixed.

Perfect, no worries!

saengel · 2024-10-07T07:05:16Z

e2e-tests/tests/interface-language-is-sticky.spec.ts

+        sourceLanguage: 'Hebrew', sourceLanguageToggle: SOURCE_LANGUAGES.HE, 
+        expectedSourceText: 'רֵאשִׁ֖ית בָּרָ֣א אֱלֹהִ֑ים אֵ֥ת הַשָּׁמַ֖יִם וְאֵ֥ת הָאָֽרֶץ׃', expectedBilingualText: '', expectedInterfaceText: 'Texts' }
+
+].forEach(({interfaceLanguage, interfaceLanguageToggle, sourceLanguage, sourceLanguageToggle, expectedSourceText, expectedBilingualText, expectedInterfaceText}) => {


Thank you for using forEach again, makes a big difference!

saengel · 2024-10-07T07:05:50Z

e2e-tests/tests/interface-language-is-sticky.spec.ts

+        }
+
+        // Validate Hebrew interface language is still toggled
+        const textLink = page.locator('a.textLink').first()


Maybe consider renaming to interfaceTextLink for clarity?

That makes sense to me.

saengel · 2024-10-07T07:07:10Z

e2e-tests/tests/translation-version-name-appears-in-title.spec.ts

+            await expect(page.locator('div.version-with-preview-title-line', {hasText: defaultTranslation!}).getByRole('link')).toHaveText(`${currentlySelected}`)
+        }
+
+        // TODO: 4th translation, handling Hebrew Interface translations in Hebrew.  For example: 'חומש רש״י, רבי שרגא זילברשטיין' should appear in the translation title as written.


Do we need to leave this TODO here? Remind me what's outstanding (I thought we were skipping this case, due to the lack of version title being displayed)

Nope, great catch!

b-w-cole added 8 commits July 18, 2024 11:31

Validate interface language is sticky when source language changes

3e60399

Merge branch 'master' into sticky-interface-and-source-language

170d3ce

Parameterized tests for brevity

c120aa6

Initial Commit of Translation Version Name in Title Bar

622d4c7

Sefaria/Sefaria-Playwright-Tests#18 TODO: Decide if we want to test for default translation title.

Testing for default source language only

353d8e9

Issue noted for Hebrew sources - no default translation is selected.

Neatening parameters

f1136b2

Updating Interface language test

0197eed

Expanding test to include different sources, plus utils addition

acb07a8

Expanding tests to handle bilingual source texts

018f556

Also allowing default translation validations to be skipped for Hebrew language source texts

b-w-cole and others added 2 commits August 9, 2024 17:06

Updated test with new global params

0bb0ec0

Solving issue with flakey tests by clearing cookies for each test

b953eaf

Issue: Tests time out on assertions looking for text in the wrong language Solution: Clearing the cookies resets the cookie-setting process and allows language cookies to be refreshed for each context.

saengel requested changes Oct 7, 2024

View reviewed changes

akiva10b and others added 3 commits October 7, 2024 13:12

chore(testing): add ip control for bilingual tests

e5131aa

Cleaned up tests to read more naturally

e1d1f38

Improving a locator for better DOM flow and readibility

111ef61

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

18 translation version name in title bar #1979

18 translation version name in title bar #1979

b-w-cole commented Jul 25, 2024

b-w-cole commented Jul 26, 2024

saengel commented Aug 1, 2024

b-w-cole commented Aug 9, 2024

b-w-cole commented Aug 9, 2024

saengel left a comment

saengel Oct 7, 2024

b-w-cole Oct 10, 2024

b-w-cole Oct 10, 2024 •

edited

Loading

b-w-cole Oct 21, 2024

saengel Oct 27, 2024

saengel Oct 7, 2024

b-w-cole Oct 10, 2024

saengel Oct 10, 2024

saengel Oct 7, 2024

b-w-cole Oct 10, 2024

saengel Oct 10, 2024

saengel Oct 7, 2024

b-w-cole Oct 10, 2024

saengel Oct 10, 2024

saengel Oct 7, 2024

saengel Oct 7, 2024

saengel Oct 7, 2024

b-w-cole Oct 10, 2024

saengel Oct 10, 2024

saengel Oct 7, 2024

saengel Oct 7, 2024

b-w-cole Oct 10, 2024

saengel Oct 10, 2024

saengel Oct 7, 2024

b-w-cole Oct 10, 2024

18 translation version name in title bar #1979

Are you sure you want to change the base?

18 translation version name in title bar #1979

Conversation

b-w-cole commented Jul 25, 2024

Description

Code Changes

Notes

b-w-cole commented Jul 26, 2024

saengel commented Aug 1, 2024

b-w-cole commented Aug 9, 2024

b-w-cole commented Aug 9, 2024

saengel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-w-cole Oct 10, 2024 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-w-cole Oct 10, 2024 •

edited

Loading