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

Возможность кастомно генерировать автоскриншоты #25

Closed
wants to merge 1 commit into from

Conversation

19thFeb
Copy link
Contributor

@19thFeb 19thFeb commented Oct 21, 2024

"@testplane/storybook": { remoteStorybookUrl: STORYBOOK_URL, customAutoScreenshots: { 'Authscreenshot default': {}, 'Autoscreenshot light theme': { globals: {theme: 'light'}}, 'Autoscreenshot dark theme': { globals: {theme: 'dark'}}, } }

image

@@ -68,6 +69,11 @@ export function parseConfig(options: PluginPartialConfig): PluginConfig {
enabled: booleanOption("enabled", true),
storybookConfigDir: stringOption("storybookConfigDir", ".storybook"),
autoScreenshots: booleanOption("autoScreenshots", true),
customAutoScreenshots: map(section({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

тут быстро не смог разобраться, как сделать так, чтобы в globals произвольный объект мог быть

Copy link
Member

Choose a reason for hiding this comment

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

import { isNull, isPlainObject } from "lodash";

const assertOptionalObject = (value, name) => {
    if (!isNull(value) && !isPlainObject(value)) {
        throw new Error(`"${name}" must be an object`);
    }
};

const optionalObject = (name, defaultValue) =>
    option({
        parseEnv: JSON.parse,
        parseCli: JSON.parse,
        defaultValue,
        validate: value => assertOptionalObject(value, name),
    });

Дефолтное значение лучше поставить {}, но тогда в src/storybook/story-test-runner/index.ts проверять нужно будет не if (customAutoScreenshots), а if (!isEmpty(customAutoScreenshots)), где isEmpty из "lodash"

Copy link
Member

Choose a reason for hiding this comment

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

Я бы вовсе сделал так, чтобы тип этого параметра был Record<string, Record<string, unknown>>. То есть, убрал этот лишний уровень вложенности с globals, потому что не имеет смысла тут в конфиге пушить пользователя описывать globals, а затем вытаскивать только этот globals, игнорируя все остальное. Если это какой-то задел на будущее, предлагаю подумать, что еще можно было бы в будущем там описывать, помимо globals. Если нет - предлагаю убрать

Copy link
Member

Choose a reason for hiding this comment

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

Если же убрать этот уровень вложенности с globals, можно будет выбрать название для опции в конфиге поудачнее. Например, autoScreenshotStorybookGlobals.

customAutoScreenshots мне не нравится во первых тем, что слишком размыто и из названия вообще непонятно, что может из себя представлять эта опция.

Copy link
Member

@KuznetsovRoman KuznetsovRoman left a comment

Choose a reason for hiding this comment

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

Выглядит прикольно.

Я бы еще поразмышлял: "может ли пользователю понадобиться иметь разные глобалы для разных историй?". Например, может ли пользователь хотеть тестировать один сторибук файл в светлой и темной теме, а другой - в двух разных локалях?

И нужно не забыть описать опцию в документации: https://github.com/gemini-testing/testplane-storybook?tab=readme-ov-file#testplane

Comment on lines +66 to +69
return browser.execute(async (globals) => {
const channel = (window as StorybookWindow).__STORYBOOK_ADDONS_CHANNEL__;
channel.emit("updateGlobals", { globals });
}, globals);
Copy link
Member

Choose a reason for hiding this comment

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

Вот тут не нужен async

@@ -68,6 +69,11 @@ export function parseConfig(options: PluginPartialConfig): PluginConfig {
enabled: booleanOption("enabled", true),
storybookConfigDir: stringOption("storybookConfigDir", ".storybook"),
autoScreenshots: booleanOption("autoScreenshots", true),
customAutoScreenshots: map(section({
Copy link
Member

Choose a reason for hiding this comment

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

import { isNull, isPlainObject } from "lodash";

const assertOptionalObject = (value, name) => {
    if (!isNull(value) && !isPlainObject(value)) {
        throw new Error(`"${name}" must be an object`);
    }
};

const optionalObject = (name, defaultValue) =>
    option({
        parseEnv: JSON.parse,
        parseCli: JSON.parse,
        defaultValue,
        validate: value => assertOptionalObject(value, name),
    });

Дефолтное значение лучше поставить {}, но тогда в src/storybook/story-test-runner/index.ts проверять нужно будет не if (customAutoScreenshots), а if (!isEmpty(customAutoScreenshots)), где isEmpty из "lodash"

@@ -68,6 +69,11 @@ export function parseConfig(options: PluginPartialConfig): PluginConfig {
enabled: booleanOption("enabled", true),
storybookConfigDir: stringOption("storybookConfigDir", ".storybook"),
autoScreenshots: booleanOption("autoScreenshots", true),
customAutoScreenshots: map(section({
Copy link
Member

Choose a reason for hiding this comment

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

Я бы вовсе сделал так, чтобы тип этого параметра был Record<string, Record<string, unknown>>. То есть, убрал этот лишний уровень вложенности с globals, потому что не имеет смысла тут в конфиге пушить пользователя описывать globals, а затем вытаскивать только этот globals, игнорируя все остальное. Если это какой-то задел на будущее, предлагаю подумать, что еще можно было бы в будущем там описывать, помимо globals. Если нет - предлагаю убрать

@@ -68,6 +69,11 @@ export function parseConfig(options: PluginPartialConfig): PluginConfig {
enabled: booleanOption("enabled", true),
storybookConfigDir: stringOption("storybookConfigDir", ".storybook"),
autoScreenshots: booleanOption("autoScreenshots", true),
customAutoScreenshots: map(section({
Copy link
Member

Choose a reason for hiding this comment

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

Если же убрать этот уровень вложенности с globals, можно будет выбрать название для опции в конфиге поудачнее. Например, autoScreenshotStorybookGlobals.

customAutoScreenshots мне не нравится во первых тем, что слишком размыто и из названия вообще непонятно, что может из себя представлять эта опция.

@19thFeb
Copy link
Contributor Author

19thFeb commented Oct 22, 2024

Я тут подумал, может вообще немного по-другому сделать?

"@testplane/storybook": {
          remoteStorybookUrl: STORYBOOK_URL,
          defaultTests: {
            'Authscreenshot default': (ctx: TestFunctionExtendedCtx) => {
              ctx.browser.assertView(...)
            },
            'Autoscreenshot first theme': (ctx: TestFunctionExtendedCtx) => {
              //set first theme
              ctx.browser.execute(...)
              ctx.browser.assertView(...)
            },
            'Autoscreenshot second theme': (ctx: TestFunctionExtendedCtx) => {
               //set second theme
              ctx.browser.execute(...)
              ctx.browser.assertView(...)
            },
        }
}

Так получается более гибко и логично, + интерфейс такой же, как у extraTests, каждый сможет делать все, что хочет(конкретно мы у себя сами проставим globals) + это можно будет переопределять на уровне конкретных сториков

@KuznetsovRoman
Copy link
Member

KuznetsovRoman commented Oct 22, 2024

Я тут подумал, может вообще немного по-другому сделать?

Мне тут не нравится, что пользователь сам вызывает assertView. testplane-storybook помимо простого вызова еще и параметры дефолтные прокидывает, и селектор, и название состояния. Тут пользователю придется самому все это описывать. Помимо того, что параметры теряются, получается еще и много бойлерплейта.

В таком случае было бы логичнее описывать "действия до скриншота", чтобы пользователю не нужно было самому этот скриншот вызывать, но:

  • Это можно сделать и просто перезаписав команду assertView через browser.overwriteCommand (тогда и параметры можно будет прозрачно пробросить)
  • Не хочется заставлять пользователя разбираться в API сторибука (через какой канал, какое событие эммитить, какие там аргументы)

Поэтому я бы предпочел упростить, насколько возможно, чтобы пользователь просто описывал сеты проверок с разными глобалами.

@19thFeb
Copy link
Contributor Author

19thFeb commented Oct 22, 2024

Я еще раз посмотрел на наши тесты, у нас везде помимо установки темы еще докидываются разные стили, поэтому, кажется, autoScreenshotStorybookGlobals - такое не подходит, так как не понятно куда их докидывать, а способ с defaultTests дает максимальную гибкость

@KuznetsovRoman
Copy link
Member

Я еще раз посмотрел на наши тесты, у нас везде помимо установки темы еще докидываются разные стили, поэтому, кажется, autoScreenshotStorybookGlobals - такое не подходит, так как не понятно куда их докидывать, а способ с defaultTests дает максимальную гибкость

Покажи пожалуйста пример. Что за стили и как они докидываются?

@19thFeb
Copy link
Contributor Author

19thFeb commented Oct 22, 2024

describe('HeaderV2', function () {
  it('Второй уровень вложенности в HeaderV2 десктоп', async function () {
    await this.browser.selectStory('header--template');
    await this.browser.setGlobals({ theme: 'dark' });
    await this.browser.addStyles('#storybook-root', { background: 'black' });
    await this.browser.$('//button[starts-with(@data-testid,"AccentDropdown-StyledButton")]//*[contains(text(), "Обучение")]').click();
    await this.browser.assertView('HeaderV2Level2Desktop', '#storybook-root');
  });

  it('Третий уровень вложенности в HeaderV2 десктоп', async function () {
    await this.browser.selectStory('header--template');
    await this.browser.setGlobals({ theme: 'light' });
    await this.browser.addStyles('#storybook-root', { background: 'light-blue' });
    await this.browser.$('//div[starts-with(@data-testid,"MenuDropdownSecondNav-RelativeDiv")]//*[contains(text(), "Сертификация")]').click();
    await this.browser.assertView('HeaderV2Level3Desktop', '#storybook-root');
  });
});
browser.addCommand(
        'addStyles',
        function (selector, styles) {
          this.execute((selector, styles, styleId) => {
            let styleElement = document.getElementById(styleId);

            if (!styleElement) {
              styleElement = document.createElement('style');
              styleElement.id = styleId;
              document.getElementsByTagName('head')[0].appendChild(styleElement);
            }

            styleElement.appendChild(document.createTextNode(`${selector} {${Object.entries(styles).map(([key, value]) => `${key}: ${value};`).join(' ')}}`));
          }, selector, styles, STYLE_ID);
        }
      );

и так во всех тестах - для каждой темы накидывается стиль

@KuznetsovRoman
Copy link
Member

и так во всех тестах - для каждой темы накидывается стиль

Для чего у #storybook-root изменяется фон? Суть этих автоскриншотных тестов - отлавливать изменения в компоненте. Если компонент внешне изменится, автоскриншотные тесты это покажут, вне зависимости от того, какой цвет у фона.

@KuznetsovRoman
Copy link
Member

Клик перед assertView тоже вызывает сомнения.
Кажется, если компонент - некая карточка, одним из элементов которой является dropdown, то нужны отдельные стори для:

  • dropdown. в состоянии "скрыт" и в состоянии "раскрыт"
  • карточки. На ней будет dropdown, но тестироваться будет только внешний вид карточки, потому что dropdown в двух состояниях уже протестирован в другой стори

@19thFeb
Copy link
Contributor Author

19thFeb commented Oct 22, 2024

Для чего у #storybook-root изменяется фон? Суть этих автоскриншотных тестов - отлавливать изменения в компоненте. Если компонент внешне изменится, автоскриншотные тесты это покажут, вне зависимости от того, какой цвет у фона.

в команде сказали, что им так проще смотреть диффы в большинстве случаев на данных фонах.

@19thFeb
Copy link
Contributor Author

19thFeb commented Oct 22, 2024

Клик перед assertView тоже вызывает сомнения. Кажется, если компонент - некая карточка, одним из элементов которой является dropdown, то нужны отдельные стори для:

  • dropdown. в состоянии "скрыт" и в состоянии "раскрыт"
  • карточки. На ней будет dropdown, но тестироваться будет только внешний вид карточки, потому что dropdown в двух состояниях уже протестирован в другой стори

Это я просто скопировал рандомные тесты, не имел в виду, что нужна возможность настраивать клик через конфиг плагина

@19thFeb
Copy link
Contributor Author

19thFeb commented Oct 22, 2024

Посыл был примерно такой: autoScreenshotStorybookGlobals - может не масштабироваться в будущем, т.к. не понятно, что делать если появятся какие-то доп условия. Мне намного больше нравится вариант с defaultTests, т.к. это просто способ изменить вот это значение по-умолчанию https://github.com/gemini-testing/testplane-storybook/blob/master/src/storybook/story-test-runner/index.ts#L22-L28 и нет ничего страшного в том, чтобы в конфиге один раз написать не 5 строчек, а 10.

@KuznetsovRoman
Copy link
Member

Посыл был примерно такой: autoScreenshotStorybookGlobals - может не масштабироваться в будущем, т.к. не понятно, что делать если появятся какие-то доп условия. Мне намного больше нравится вариант с defaultTests, т.к. это просто способ изменить вот это значение по-умолчанию https://github.com/gemini-testing/testplane-storybook/blob/master/src/storybook/story-test-runner/index.ts#L22-L28

Если хочется максимальной масштабируемости и возможности самостоятельно писать такие тесты и многие другие, советую посмотреть в эту сторону: https://github.com/gemini-testing/testplane-storybook/?tab=readme-ov-file#advanced-usage

Загрузка истории будет выполняться на стороне testplane-storybook, а вы сможете самостоятельно описывать в этих тестах необходимые действия. Можно даже выключить автоскриншотные тесты сторибука и в testplane секции сторей описывать все тесты самостоятельно:

import defaultTestplaneTests from "...";

import type { StoryObj } from "@storybook/react";
import type { WithTestplane } from "@testplane/storybook"

export const Primary: WithTestplane<StoryObj> = {
    args: {
        primary: true,
        label: "Button",
    },
    testplane: defaultTestplaneTests,
};

export const Secondary: WithTestplane<StoryObj> = {
    args: {
        primary: false,
        label: "Button",
    },
    testplane: {
        ...defaultTestplaneTests,
        "my test": async ({browser, expect}) => {
            const element = await browser.$(".storybook-button");

            await expect(element).toHaveText("Button");
        }
    },
};

Это гибко и масштабируемо, потому что можно описывать тесты для каждой стори отдельно от других.
При подходе с глобальными тестами масштабируемость теряется, потому что несомненно возникнет какая-либо сторя, для которой эти дефолтные тесты выполнять не нужно, и тогда придется придумывать, что делать уже с этой проблемой (в особенности если ты при этом еще хочешь вызывать бойлерплейтную assertView с неочевидными для пользователя аргументами)

Если же просто хочется поменять фон сторибука, то для этого не нужно изменять дефолтные testplane тесты. Можно сделать это средствами самого сторибука: их можно указать как на глобальном уровне, на уровне компонента и на уровне стори: https://storybook.js.org/docs/writing-stories/parameters#global-parameters

Еще можно написать декоратор: https://storybook.js.org/docs/writing-stories/decorators#global-decorators

Или можно использовать сторибук аддон backgrounds: https://storybook.js.org/docs/essentials/backgrounds
Тут вроде есть возможность изменять фон с помощью globals

Autoscreenshot все же предназначен для простых неконфигурируемых автоскриншотных тестов, не хочется это лишний раз усложнять, тем более, что для сложных кастомных сценариев, опять же, уже есть решение - кастомные тесты в testplane секции.

@19thFeb
Copy link
Contributor Author

19thFeb commented Oct 22, 2024

Конкретно сейчас мне хочется тестировать автоматически компоненты в двух темах(чтобы генерировалось 2 автоскриншотных теста, а не 1) - сейчас такой возможности нет. Про advanced-usage я в курсе, но для этого будет нужно в каждой истории писать эти дополнительные тесты

@19thFeb
Copy link
Contributor Author

19thFeb commented Oct 22, 2024

При подходе с глобальными тестами масштабируемость теряется, потому что несомненно возникнет какая-либо сторя, для которой эти дефолтные тесты выполнять не нужно, и тогда придется придумывать, что делать уже с этой проблемой (в особенности если ты при этом еще хочешь вызывать бойлерплейтную assertView с неочевидными для пользователя аргументами)

Вот это немного не понял, параметр defaultTests ничего не ломает же, параметр skip для него будет работать так же как и раньше - просто не сгенерируются тесты

@KuznetsovRoman
Copy link
Member

KuznetsovRoman commented Oct 25, 2024

В общем, идея с добавлением в конфиг чего-то типа "autoScreenshotStorybookGlobals", чтобы из одной стори делать сразу несколько скриншотных тестов нам нравится.

Вижу, в коде по ключу с "названием теста" есть объект с одним полем: globals. Есть ли идеи, какие ключи там еще могут понадобиться, помимо globals? Если нет, то не вижу смысла в том, чтобы заставлять пользователя явно описывать этот дополнительный уровень вложенности.

Касаемо доп. стилей: изменения цвета фона можно (и лучше) добиться с помощью аддонов под сторибук.

Про defaultTests: это потенциально может привести пользователей к ошибкам, затруднит пользование плагином для пользователей и затруднит поддержку плагина (в том числе добавление новых фич) в будущем. Раз описанные проблемы можно решить и без этого, и иных заказчиков у такой фичи нет, пока это принесет больше гарантированных проблем, чем потенциальной пользы.

По этому PR: мы можем сами закончить работу над этой фичей (с возможностью декларативно указывать разные сеты глобалов), нам так, наверное, будет проще. Или есть сильное желание сделать это самостоятельно?

@19thFeb
Copy link
Contributor Author

19thFeb commented Oct 25, 2024

Вижу, в коде по ключу с "названием теста" есть объект с одним полем: globals. Есть ли идеи, какие ключи там еще могут понадобиться, помимо globals? Если нет, то не вижу смысла в том, чтобы заставлять пользователя явно описывать этот дополнительный уровень вложенности.

У меня нет, у меня не очень много опыта со сторибуком, поэтому я скорее перестраховался для будущего возможного расширения.

По этому PR: мы можем сами закончить работу над этой фичей (с возможностью декларативно указывать разные сеты глобалов), нам так, наверное, будет проще. Или есть сильное желание сделать это самостоятельно?

Если вам так будет проще, то давайте так, это непринципиально

@KuznetsovRoman
Copy link
Member

По этому PR: мы можем сами закончить работу над этой фичей (с возможностью декларативно указывать разные сеты глобалов), нам так, наверное, будет проще. Или есть сильное желание сделать это самостоятельно?

Если вам так будет проще, то давайте так, это непринципиально

Доработал PR с твоим коммитом в #28, этот закрываю. Спасибо за контрибьют!

@19thFeb
Copy link
Contributor Author

19thFeb commented Nov 7, 2024

Отлично! Спасибо и вам

@KuznetsovRoman
Copy link
Member

Отлично! Спасибо и вам

Released as @testplane/[email protected]

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