ronantakizawa/codereview-bench huggingface dataset:
Repository: apache/airflow
File: airflow-core/src/airflow/ui/tests/e2e/specs/dag-audit-log.spec.ts (TypeScript)

Code before:
        const rows = table.querySelectorAll("tbody tr");

        return rows.length >= minCount;
      },
      expectedEventCount,
      { timeout: 60_000 },
    );

    await context.close();
  });

  test.beforeEach(({ page }) => {
    eventsPage = new EventsPage(page);
  });

  test("should navigate to audit log tab and display table", async () => {
    await eventsPage.navigateToAuditLog(testDagId);

    await expect(eventsPage.eventsTable).toBeVisible();

    const rowCount = await eventsPage.tableRows.count();

    expect(rowCount).toBeGreaterThan(0);
  });

  test("should display all expected columns in audit log table", async () => {
    await eventsPage.navigateToAuditLog(testDagId);

    await expect(eventsPage.whenColumn).toBeVisible();
    await expect(eventsPage.eventColumn).toBeVisible();
    await expect(eventsPage.ownerColumn).toBeVisible();
    await expect(eventsPage.extraColumn).toBeVisible();

    const dagIdColumn = eventsPage.eventsTable.locator('th:has-text("DAG ID")');

    await expect(dagIdColumn).not.toBeVisible();
  });

  test("should display audit log entries with valid data", async () => {
    await eventsPage.navigateToAuditLog(testDagId);

    const rows = await eventsPage.getEventLogRows();

    expect(rows.length).toBeGreaterThan(0);

    const [firstRow] = rows;

    if (!firstRow) {
      throw new Error("No rows found");
    }


Review comment (suggestion):
rename test to `verify expected columns are visible`

Code after:
        const rows = table.querySelectorAll("tbody tr");

        return rows.length >= minCount;
      },
      expectedEventCount,
      { timeout: 60_000 },
    );

    await context.close();
  });

  test.beforeEach(({ page }) => {
    eventsPage = new EventsPage(page);
  });

  test("verify audit log table displays", async () => {
    await eventsPage.navigateToAuditLog(testDagId);

    await expect(eventsPage.eventsTable).toBeVisible();

    const rowCount = await eventsPage.tableRows.count();

    expect(rowCount).toBeGreaterThan(0);
  });

  test("verify expected columns are visible", async () => {
    await eventsPage.navigateToAuditLog(testDagId);

    await expect(eventsPage.whenColumn).toBeVisible();
    await expect(eventsPage.eventColumn).toBeVisible();
    await expect(eventsPage.ownerColumn).toBeVisible();
    await expect(eventsPage.extraColumn).toBeVisible();

    const dagIdColumn = eventsPage.eventsTable.locator('th:has-text("DAG ID")');

    await expect(dagIdColumn).not.toBeVisible();
  });

  test("verify audit log entries display valid data", async () => {
    await eventsPage.navigateToAuditLog(testDagId);

    const rows = await eventsPage.getEventLogRows();

    expect(rows.length).toBeGreaterThan(0);

    const [firstRow] = rows;

    if (!firstRow) {
      throw new Error("No rows found");
    }
Repository: apache/airflow
File: airflow-core/src/airflow/ui/tests/e2e/specs/dag-audit-log.spec.ts (TypeScript)

Code before:

    const hasNext = await eventsPage.hasNextPage();

    expect(hasNext).toBe(true);

    const urlPage1 = eventsPage.page.url();

    expect(urlPage1).toContain("offset=0");
    expect(urlPage1).toContain("limit=3");

    await eventsPage.clickNextPage();

    const urlPage2 = eventsPage.page.url();

    expect(urlPage2).toContain("limit=3");
    expect(urlPage2).not.toContain("offset=0");

    await eventsPage.clickPrevPage();

    const urlBackToPage1 = eventsPage.page.url();

    expect(urlBackToPage1).toContain("offset=0");
    expect(urlBackToPage1).toContain("limit=3");
  });

  test("should sort audit log entries when clicking column header", async () => {
    await eventsPage.navigateToAuditLog(testDagId);

    const initialEvents = await eventsPage.getEventTypes(true);

    await eventsPage.clickColumnToSort("Event");

    const sortedEvents = await eventsPage.getEventTypes(true);

    const expectedSorted = [...initialEvents].sort();

    expect(sortedEvents).toEqual(expectedSorted);
  });
});


Review comment (suggestion):
Test name should be `verify sorting when clicking column header`

Code after:

    expect(hasNext).toBe(true);

    const urlPage1 = eventsPage.page.url();

    expect(urlPage1).toContain("offset=0");
    expect(urlPage1).toContain("limit=3");

    await eventsPage.clickNextPage();

    const urlPage2 = eventsPage.page.url();

    expect(urlPage2).toContain("limit=3");
    expect(urlPage2).not.toContain("offset=0");

    await eventsPage.clickPrevPage();

    const urlBackToPage1 = eventsPage.page.url();

    expect(urlBackToPage1).toContain("offset=0");
    expect(urlBackToPage1).toContain("limit=3");
  });

  test("verify sorting when clicking column header", async () => {
    await eventsPage.navigateToAuditLog(testDagId);

    const initialEvents = await eventsPage.getEventTypes(true);

    await eventsPage.clickColumnToSort("Event");

    const sortedEvents = await eventsPage.getEventTypes(true);

    const expectedSorted = [...initialEvents].sort();

    expect(sortedEvents).toEqual(expectedSorted);
  });
});
Repository: apache/airflow
File: airflow-core/src/airflow/ui/tests/e2e/specs/dag-audit-log.spec.ts (TypeScript)

Code before:

    if (!firstRow) {
      throw new Error("No rows found");
    }

    const cells = firstRow.locator("td");

    const whenCell = cells.nth(0);
    const eventCell = cells.nth(1);
    const userCell = cells.nth(2);

    const whenText = await whenCell.textContent();
    const eventText = await eventCell.textContent();
    const userText = await userCell.textContent();

    expect(whenText).toBeTruthy();
    expect(whenText?.trim()).not.toBe("");

    expect(eventText).toBeTruthy();
    expect(eventText?.trim()).not.toBe("");

    expect(userText).toBeTruthy();
    expect(userText?.trim()).not.toBe("");
  });

  test("should paginate through audit log entries", async () => {
    await eventsPage.navigateToAuditLog(testDagId, 3);

    const hasNext = await eventsPage.hasNextPage();

    expect(hasNext).toBe(true);

    const urlPage1 = eventsPage.page.url();

    expect(urlPage1).toContain("offset=0");
    expect(urlPage1).toContain("limit=3");

    await eventsPage.clickNextPage();

    const urlPage2 = eventsPage.page.url();

    expect(urlPage2).toContain("limit=3");
    expect(urlPage2).not.toContain("offset=0");

    await eventsPage.clickPrevPage();

    const urlBackToPage1 = eventsPage.page.url();

    expect(urlBackToPage1).toContain("offset=0");
    expect(urlBackToPage1).toContain("limit=3");
  });

Review comment (suggestion):
rename to `verify pagination through audit log entries`

Code after:

    if (!firstRow) {
      throw new Error("No rows found");
    }

    const whenCell = await eventsPage.getCellByColumnName(firstRow, "When");
    const eventCell = await eventsPage.getCellByColumnName(firstRow, "Event");
    const userCell = await eventsPage.getCellByColumnName(firstRow, "User");

    const whenText = await whenCell.textContent();
    const eventText = await eventCell.textContent();
    const userText = await userCell.textContent();

    expect(whenText).toBeTruthy();
    expect(whenText?.trim()).not.toBe("");

    expect(eventText).toBeTruthy();
    expect(eventText?.trim()).not.toBe("");

    expect(userText).toBeTruthy();
    expect(userText?.trim()).not.toBe("");
  });

  test("verify pagination through audit log entries", async () => {
    await eventsPage.navigateToAuditLog(testDagId, 3);

    const hasNext = await eventsPage.hasNextPage();

    expect(hasNext).toBe(true);

    const urlPage1 = eventsPage.page.url();

    expect(urlPage1).toContain("offset=0");
    expect(urlPage1).toContain("limit=3");

    await eventsPage.clickNextPage();

    const urlPage2 = eventsPage.page.url();

    expect(urlPage2).toContain("limit=3");
    expect(urlPage2).not.toContain("offset=0");

    await eventsPage.clickPrevPage();

    const urlBackToPage1 = eventsPage.page.url();

    expect(urlBackToPage1).toContain("offset=0");
    expect(urlBackToPage1).toContain("limit=3");
  });

  test("verify sorting when clicking column header", async () => {
Repository: apache/airflow
File: airflow-core/src/airflow/ui/tests/e2e/specs/dag-runs.spec.ts (TypeScript)

Code before:
import { test } from "@playwright/test";
import { DagRunsPage } from "tests/e2e/pages/DagRunsPage";

test.describe("DAG Runs Page", () => {
  let dagRunsPage: DagRunsPage;

  test.beforeEach(({ page }) => {
    dagRunsPage = new DagRunsPage(page);
  });

  test("should display the DAG runs table with data", async () => {
    await dagRunsPage.navigate();
    await dagRunsPage.verifyDagRunsExist();
  });

  test("should display run details: run ID, state, and dates", async () => {
    await dagRunsPage.navigate();
    await dagRunsPage.verifyRunDetailsDisplay();
  });

  test("should display different run states with visual badges", async () => {
    await dagRunsPage.navigate();
    await dagRunsPage.verifyStateVisualDistinction();
  });

  test("should have filtering functionality", async () => {
    await dagRunsPage.navigate();
    await dagRunsPage.verifyFilteringWorks();
  });
});


Review comment (bug):
As mentioned [here](https://github.com/apache/airflow/pull/59919#issuecomment-3713306224). We should filter and check whether filter is applied correctly or not. Let's filter with dag_id(Create runs for different dag_id in beforeAll) and state(create run with failed state in before all )

Code after:
import { test, expect } from "@playwright/test";
import { AUTH_FILE, testConfig } from "playwright.config";
import { DagRunsPage } from "tests/e2e/pages/DagRunsPage";

test.describe("DAG Runs Page", () => {
  test.setTimeout(60_000);

  let dagRunsPage: DagRunsPage;
  const testDagId1 = testConfig.testDag.id;
  const testDagId2 = "example_python_operator";

  test.beforeAll(async ({ browser }) => {
    const context = await browser.newContext({ storageState: AUTH_FILE });
    const page = await context.newPage();
    const baseUrl = process.env.AIRFLOW_UI_BASE_URL ?? "http://localhost:8080";

    const timestamp = Date.now();

    // Trigger first DAG run and mark as failed
    const runId1 = `test_run_failed_${timestamp}`;
    const logicalDate1 = new Date(timestamp).toISOString();
    const triggerResponse1 = await page.request.post(`${baseUrl}/api/v2/dags/${testDagId1}/dagRuns`, {
      data: JSON.stringify({
        dag_run_id: runId1,
        logical_date: logicalDate1,
      }),
      headers: {
        "Content-Type": "application/json",
      },
    });

    expect(triggerResponse1.ok()).toBeTruthy();
    const runData1 = (await triggerResponse1.json()) as { dag_run_id: string };

    // Mark the first run as failed
    const patchResponse1 = await page.request.patch(
      `${baseUrl}/api/v2/dags/${testDagId1}/dagRuns/${runData1.dag_run_id}`,
      {
        data: JSON.stringify({ state: "failed" }),
        headers: {
          "Content-Type": "application/json",
        },
      },
    );

    expect(patchResponse1.ok()).toBeTruthy();

    // Trigger second DAG run and mark as success
    const runId2 = `test_run_success_${timestamp}`;
    const logicalDate2 = new Date(timestamp + 60_000).toISOString();
    const triggerResponse2 = await page.request.post(`${baseUrl}/api/v2/dags/${testDagId1}/dagRuns`, {
Repository: apache/airflow
File: airflow-core/src/airflow/ui/tests/e2e/pages/AssetListPage.ts (TypeScript)

Code before:
import type { Locator, Page } from "@playwright/test";

import { BasePage } from "./BasePage";

export class AssetListPage extends BasePage {
  public readonly emptyState: Locator;
  public readonly heading: Locator;
  public readonly nextButton: Locator;
  public readonly prevButton: Locator;
  public readonly rows: Locator;
  public readonly searchInput: Locator;
  public readonly table: Locator;

  public constructor(page: Page) {
    super(page);

    this.heading = page.getByRole("heading", { level: 2 });
    this.table = page.getByTestId("table-list");
    this.rows = this.table.locator("tbody tr").filter({
      has: page.locator("td"),
    });

    this.nextButton = page.getByTestId("next");
    this.prevButton = page.getByTestId("prev");

    this.searchInput = page.getByRole("textbox");
    this.emptyState = page.getByText(/no items/i);
  }

  public async assetCount(): Promise<number> {
    return this.rows.count();
  }

  public async assetNames(): Promise<Array<string>> {
    return this.rows.locator("td a").allTextContents();
  }

  public async goNext(): Promise<boolean> {
    const next = this.page.locator('[data-testid="next"]');

    if ((await next.count()) === 0) {
      return false;
    }

    await next.click();

    return true;
  }

  public async hasPagination(): Promise<boolean> {
    return (await this.page.locator('[data-testid="next"]').count()) > 0;

Review comment (suggestion):
We can use `this.searchInput = page.getByTestId("search-dags");`

Code after:
import type { Locator, Page } from "@playwright/test";

import { BasePage } from "./BasePage";

export class AssetListPage extends BasePage {
  public readonly emptyState: Locator;
  public readonly heading: Locator;
  public readonly rows: Locator;
  public readonly searchInput: Locator;
  public readonly table: Locator;

  public constructor(page: Page) {
    super(page);

    this.heading = page.getByRole("heading", {
      name: /\d+\s+asset/i,
    });
    this.table = page.getByTestId("table-list");
    this.rows = this.table.locator("tbody tr").filter({
      has: page.locator("td"),
    });

    this.searchInput = page.getByTestId("search-dags");
    this.emptyState = page.getByText(/no items/i);
  }

  public async assetCount(): Promise<number> {
    return this.rows.count();
  }

  public async assetNames(): Promise<Array<string>> {
    return this.rows.locator("td a").allTextContents();
  }

  public async navigate(): Promise<void> {
    await this.navigateTo("/assets");
  }

  public async openFirstAsset(): Promise<string> {
    const count = await this.rows.count();

    if (count === 0) {
      throw new Error("No assets found to click");
    }

    const link = this.rows.nth(0).locator("a").first();
    const name = await link.textContent();

    await link.click();

    return name?.trim() ?? "";
Repository: apache/airflow
File: providers/apache/spark/tests/system/apache/spark/example_pyspark.py (Python)

Code before:
if typing.TYPE_CHECKING:
    import pandas as pd
    from pyspark.sql import SparkSession

try:
    from airflow.sdk import dag, task
except ImportError:
    # Airflow 2 path
    from airflow.decorators import dag, task  # type: ignore[attr-defined,no-redef]


@dag(
    schedule=None,
    start_date=pendulum.datetime(2021, 1, 1, tz="UTC"),
    catchup=False,
    tags=["example"],
)
def example_pyspark():
    """
    ### Example Pyspark DAG
    This is an example DAG which uses pyspark
    """

    # [START task_pyspark]
    @task.pyspark(conn_id="spark-local")
    def spark_task(spark: SparkSession, sc) -> pd.DataFrame:
        df = spark.createDataFrame(
            [
                (1, "John Doe", 21),
                (2, "Jane Doe", 22),
                (3, "Joe Bloggs", 23),
            ],
            ["id", "name", "age"],
        )
        df.show()

        return df.toPandas()

    # [END task_pyspark]

    @task
    def print_df(df: pd.DataFrame):
        print(df)

    df = spark_task()
    print_df(df)


# work around mypy
dag = example_pyspark()  # type: ignore


Review comment (suggestion):
The second argument can be named better, and maybe have a type annotation.

Code after:
if typing.TYPE_CHECKING:
    import pandas as pd
    from pyspark.sql import SparkSession

try:
    from airflow.sdk import dag, task
except ImportError:
    # Airflow 2 path
    from airflow.decorators import dag, task  # type: ignore[attr-defined,no-redef]


@dag(
    schedule=None,
    start_date=pendulum.datetime(2021, 1, 1, tz="UTC"),
    catchup=False,
    tags=["example"],
)
def example_pyspark():
    """
    ### Example Pyspark DAG
    This is an example DAG which uses pyspark
    """

    # [START task_pyspark]
    @task.pyspark(conn_id="spark-local")
    def spark_task(spark: SparkSession) -> pd.DataFrame:
        df = spark.createDataFrame(
            [
                (1, "John Doe", 21),
                (2, "Jane Doe", 22),
                (3, "Joe Bloggs", 23),
            ],
            ["id", "name", "age"],
        )
        df.show()

        return df.toPandas()

    # [END task_pyspark]

    @task
    def print_df(df: pd.DataFrame):
        print(df)

    df = spark_task()
    print_df(df)


# work around mypy
dag = example_pyspark()  # type: ignore
Repository: apache/airflow
File: scripts/ci/prek/check_notice_files.py (Python)

Code before:
- Include the current year in copyright statements
- Reference the Apache Software Foundation

Usage: check_notice_files.py <notice_file_paths...>
"""

from __future__ import annotations

import sys
from datetime import datetime
from pathlib import Path

CURRENT_YEAR = str(datetime.now().year)

errors = 0

for notice_file in sys.argv[1:]:
    content = Path(notice_file).read_text()

    if "Copyright" in content and CURRENT_YEAR not in content:
        print(f"❌ {notice_file}: Missing current year ({CURRENT_YEAR}) in copyright")
        errors += 1

    if "Apache Software Foundation" not in content:
        print(f"❌ {notice_file}: Missing 'Apache Software Foundation' reference")
        errors += 1

sys.exit(1 if errors else 0)


Review comment (suggestion):
I just found that the format is `Copyright 2016-{CURRENT_YEAR} The Apache Software Foundation`, not sure whether it would be bette to use f-string approach.

```suggestion
    expected = f"Copyright 2016-{CURRENT_YEAR} The Apache Software Foundation"
    if "Copyright" in content and expected not in content:
        print(f"❌ {notice_file}: Missing expected string: {expected!r}")
        errors += 1
```

Code after:
- Include the current year in copyright statements
- Reference the Apache Software Foundation

Usage: check_notice_files.py <notice_file_paths...>
"""

from __future__ import annotations

import sys
from datetime import datetime
from pathlib import Path

CURRENT_YEAR = str(datetime.now().year)

errors = 0

for notice_file in sys.argv[1:]:
    content = Path(notice_file).read_text()

    expected = f"Copyright 2016-{CURRENT_YEAR} The Apache Software Foundation"
    if "Copyright" in content and expected not in content:
        print(f"❌ {notice_file}: Missing expected string: {expected!r}")
        errors += 1

sys.exit(1 if errors else 0)
Repository: apache/airflow
File: task-sdk/src/airflow/sdk/__init__.py (Python)

Code before:
    "conf": ".configuration",
    "cross_downstream": ".bases.operator",
    "dag": ".definitions.dag",
    "get_current_context": ".definitions.context",
    "get_parsing_context": ".definitions.context",
    "literal": ".definitions.template",
    "macros": ".execution_time.macros",
    "setup": ".definitions.decorators",
    "task": ".definitions.decorators",
    "task_group": ".definitions.decorators",
    "teardown": ".definitions.decorators",
}


def __getattr__(name: str):
    if module_path := __lazy_imports.get(name):
        import importlib

        mod = importlib.import_module(module_path, __name__)

        # try and get the attribute from the module
        # if it doesn't exist (for modules like macros), return module itself
        try:
            val = getattr(mod, name)
        except AttributeError:
            val = mod

        # Store for next time
        globals()[name] = val
        return val
    raise AttributeError(f"module {__name__!r} has no attribute {name!r}")


Review comment (suggestion):
I'm a bit concerned of this workaround, as it might backfire in potential future namespace-naming conflicts. I'd maybe limit it only to the modules that require this:

```suggestion
        # try and get the attribute from the module
        # if it doesn't exist (for modules like macros), return module itself
        try:
            val = getattr(mod, name)
        except AttributeError:
            if name == "macros":
                val = mod
            else:
                raise
```

Code after:
    "conf": ".configuration",
    "cross_downstream": ".bases.operator",
    "dag": ".definitions.dag",
    "get_current_context": ".definitions.context",
    "get_parsing_context": ".definitions.context",
    "literal": ".definitions.template",
    "macros": ".execution_time",
    "setup": ".definitions.decorators",
    "task": ".definitions.decorators",
    "task_group": ".definitions.decorators",
    "teardown": ".definitions.decorators",
}


def __getattr__(name: str):
    if module_path := __lazy_imports.get(name):
        import importlib

        mod = importlib.import_module(module_path, __name__)
        val = getattr(mod, name)

        # Store for next time
        globals()[name] = val
        return val
    raise AttributeError(f"module {__name__!r} has no attribute {name!r}")
Repository: apache/airflow
File: airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_pools.py (Python)

Code before:
pytestmark = pytest.mark.db_test

POOL1_NAME = "pool1"
POOL1_SLOT = 3
POOL1_INCLUDE_DEFERRED = True


POOL2_NAME = "pool2"
POOL2_SLOT = 10
POOL2_INCLUDE_DEFERRED = False
POOL2_DESCRIPTION = "Some Description"


POOL3_NAME = "pool3/with_slashes"
POOL3_SLOT = 5
POOL3_INCLUDE_DEFERRED = False
POOL3_DESCRIPTION = "Some Description"


@provide_session
def _create_pools(session) -> None:
    team = session.scalars(select(Team).where(Team.name == "test")).one()

    pool1 = Pool(
        pool=POOL1_NAME, slots=POOL1_SLOT, include_deferred=POOL1_INCLUDE_DEFERRED, team_name=team.name
    )
    pool2 = Pool(pool=POOL2_NAME, slots=POOL2_SLOT, include_deferred=POOL2_INCLUDE_DEFERRED)
    pool3 = Pool(
        pool=POOL3_NAME,
        slots=POOL3_SLOT,
        include_deferred=POOL3_INCLUDE_DEFERRED,
        description=POOL3_DESCRIPTION,
    )
    session.add_all([pool1, pool2, pool3])


@provide_session
def _create_team(session) -> None:
    session.add(Team(name="test"))
    session.commit()


class TestPoolsEndpoint:
    @pytest.fixture(autouse=True)
    def setup(self) -> None:
        clear_db_pools()
        clear_db_teams()

    def teardown_method(self) -> None:
        clear_db_pools()


Review comment (suggestion):
It’s not necessary to change this, but it seems we don’t need to select the team before creating a pool.⁠

```suggestion
    pool1 = Pool(
        pool=POOL1_NAME, slots=POOL1_SLOT, include_deferred=POOL1_INCLUDE_DEFERRED, team_name="test"
    )
```

Code after:
pytestmark = pytest.mark.db_test

POOL1_NAME = "pool1"
POOL1_SLOT = 3
POOL1_INCLUDE_DEFERRED = True


POOL2_NAME = "pool2"
POOL2_SLOT = 10
POOL2_INCLUDE_DEFERRED = False
POOL2_DESCRIPTION = "Some Description"


POOL3_NAME = "pool3/with_slashes"
POOL3_SLOT = 5
POOL3_INCLUDE_DEFERRED = False
POOL3_DESCRIPTION = "Some Description"


@provide_session
def _create_pools(session) -> None:
    pool1 = Pool(pool=POOL1_NAME, slots=POOL1_SLOT, include_deferred=POOL1_INCLUDE_DEFERRED, team_name="test")
    pool2 = Pool(pool=POOL2_NAME, slots=POOL2_SLOT, include_deferred=POOL2_INCLUDE_DEFERRED)
    pool3 = Pool(
        pool=POOL3_NAME,
        slots=POOL3_SLOT,
        include_deferred=POOL3_INCLUDE_DEFERRED,
        description=POOL3_DESCRIPTION,
    )
    session.add_all([pool1, pool2, pool3])


@provide_session
def _create_team(session) -> None:
    session.add(Team(name="test"))
    session.commit()


class TestPoolsEndpoint:
    @pytest.fixture(autouse=True)
    def setup(self) -> None:
        clear_db_pools()
        clear_db_teams()

    def teardown_method(self) -> None:
        clear_db_pools()

    def create_pools(self):
        _create_team()
        _create_pools()