Skip to content

Conversation

@qamarq
Copy link
Member

@qamarq qamarq commented Jul 24, 2025

Lokalnie śmiga
Tylko funkcja wybierania wydziału nie działa na githubie, będę debugowal żeby śmigało też na gicie

@qamarq qamarq marked this pull request as ready for review July 25, 2025 21:59
test("submit incorrect email", async ({ page }) => {
await fillEmailInput(page, "[email protected]");

await expect(page.getByText(/il musi być z domeny Polite/i)).toBeVisible();
Copy link

Choose a reason for hiding this comment

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

The regex for the email domain error message seems vague. Consider matching the full expected error text to avoid false positives.

Suggested change
await expect(page.getByText(/il musi być z domeny Polite/i)).toBeVisible();
await expect(page.getByText(/Email musi być z domeny Politechniki Wrocławskiej/i)).toBeVisible();

Copy link
Member

Choose a reason for hiding this comment

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

ma rację, nie wiem jaki jest sens dawania tylko częsci - dla mnie to jest confusing

import { Page, expect } from "@playwright/test";

export const selectFacultyAndRegistration = async (page: Page) => {
console.log("selectFacultyAndRegistration - 1");
Copy link

Choose a reason for hiding this comment

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

There are several console.log statements in the test helper. Remove or limit debug logs to keep test output clean.

Suggested change
console.log("selectFacultyAndRegistration - 1");

Copy link
Member

Choose a reason for hiding this comment

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

fakty^

await emailInput.press("Enter");

await expect(page.getByText(/tuł musi mieć/i)).toBeVisible();
await expect(page.getByText(/pis musi mieć co/i)).toBeVisible();
Copy link

Choose a reason for hiding this comment

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

Typographical error: The expected text 'pis musi mieć co' likely should be 'opis musi mieć co najmniej ...' (depending on full context).

Suggested change
await expect(page.getByText(/pis musi mieć co/i)).toBeVisible();
await expect(page.getByText(/opis musi mieć co najmniej/i)).toBeVisible();

Copy link
Member

Choose a reason for hiding this comment

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

ma racje

Copy link
Member

@Rei-x Rei-x left a comment

Choose a reason for hiding this comment

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

spoczko testy, testują co mają testować i nawet ładnie, że wszystkie mogą lecieć in parallel i są od siebie niezależne. dodatkowy props za stawianie api lokalnie i używanie go, sporo osób używało produkcyjnego

z rzeczy do poprawy to są locatory do rzeczy, w niektórych miejscach bardzo słabe, pewnie też to wynika z tego, że dostępność planera leży, wypadałoby to poprawić

poza tym wszystko jest nawet spoczko, poczytaj sobie też jak zrobić autha w playwrighcie, żeby nie mieć niespodzianek: https://playwright.dev/docs/auth

build:
name: Test build
runs-on: ubuntu-latest
needs: [lint, prettier, deadcode]
Copy link
Member

Choose a reason for hiding this comment

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

po co to w sumie jest? raczej chcemy mieć jak najszybciej feedback, a nie że muszę czekać na formatowanie żeby wiedzieć czy moja apka się builduje

- uses: actions/checkout@v4
- uses: actions/setup-node@v4
with:
node-version: lts/*
Copy link
Member

Choose a reason for hiding this comment

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

raczej chcemy wersje z repo, a nie obecną najnowszą

import { Page, expect } from "@playwright/test";

export const selectFacultyAndRegistration = async (page: Page) => {
console.log("selectFacultyAndRegistration - 1");
Copy link
Member

Choose a reason for hiding this comment

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

fakty^


export const selectFacultyAndRegistration = async (page: Page) => {
console.log("selectFacultyAndRegistration - 1");
await page.waitForTimeout(1000);
Copy link
Member

Choose a reason for hiding this comment

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

no raczej nie powinno się takich rzeczy umieszczać w testach

page,
}) => {
await page.route(
"http://localhost:3333/departments/W5/registrations/W05-APR-SI-3-25Z/courses",
Copy link
Member

Choose a reason for hiding this comment

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

zahardkodowany localhost, raczej tak nie powinno być


await selectFacultyAndRegistration(page);

await page.waitForTimeout(2000);
Copy link
Member

Choose a reason for hiding this comment

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

nie powinno się używać timeoutów

});
});

test.describe("Report form tests", () => {
Copy link
Member

Choose a reason for hiding this comment

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

wrzuciłbym to do oddzielnego pliku, to raczej osobna rzecz do przetestowania

await emailInput.press("Enter");

await expect(page.getByText(/tuł musi mieć/i)).toBeVisible();
await expect(page.getByText(/pis musi mieć co/i)).toBeVisible();
Copy link
Member

Choose a reason for hiding this comment

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

ma racje

selectFacultyAndRegistration,
} from "./helpers";

test.describe("Plans logic online", () => {
Copy link
Member

Choose a reason for hiding this comment

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

nie sprawdzam tego testu, bo wygląda mocno WIP

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants