Skip to content

Conversation

@neha00290
Copy link
Contributor

@neha00290 neha00290 commented Sep 2, 2025

PR Type

Tests, Enhancement


Description

  • Add logs table field management tests

  • Extend LogsPage POM with field actions

  • Integrate new test into Playwright workflow

  • Include ingestion helper for test setup


Diagram Walkthrough

flowchart LR
  POM["LogsPage POM"]
  Tests["logstable.spec.js"]
  CI["playwright.yml matrix"]
  Ingest["Ingestion helper"]

  Tests -- "uses" --> POM
  Tests -- "posts logs" --> Ingest
  CI -- "runs" --> Tests
Loading

File Walkthrough

Relevant files
Enhancement
logsPage.js
POM: field add/remove helpers for logs table                         

tests/ui-testing/pages/logsPages/logsPage.js

  • Add hover, add, remove field helpers
  • Add table header visibility assertions
  • Introduce negative assertion via source column
  • Minor waits for UI stability
+25/-0   
Tests
logstable.spec.js
Add logs table field management E2E tests                               

tests/ui-testing/playwright-tests/Logs/logstable.spec.js

  • New E2E tests for add/remove field
  • Test for field persistence after refresh
  • Ingestion helper posts logs before tests
  • Setup ensures quick mode is off
+149/-0 
Configuration changes
playwright.yml
CI: include logs table tests in workflow                                 

.github/workflows/playwright.yml

  • Add logstable.spec.js to Logs-Core matrix
  • Ensure CI runs new tests
+2/-1     

@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2025

PR Reviewer Guide 🔍

(Review updated until commit 7dc3f92)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 Security concerns

Sensitive information handling:
The test builds a Basic Authorization header from environment variables and logs the ingestion response and errors. Ensure responses do not contain sensitive data before logging, and consider masking headers or avoiding logging raw responses.

⚡ Recommended focus areas for review

Flaky Timing

Multiple fixed waitForTimeout calls after UI actions may cause flaky tests; prefer waiting on specific locators or assertions that reflect the expected state change.

    await this.page.locator(`[data-test="log-search-expand-${fieldName}-field-btn"]`).hover();
    await this.page.waitForTimeout(300);
}

async clickAddFieldToTableButton(fieldName) {
    await this.page.locator(`[data-test="log-search-index-list-add-${fieldName}-field-btn"]`).click();
    await this.page.waitForTimeout(1000);
}

async clickRemoveFieldFromTableButton(fieldName) {
    await this.page.locator(`[data-test="log-search-index-list-remove-${fieldName}-field-btn"]`).click();
    await this.page.waitForTimeout(1000);
}
Secret Usage

Basic auth credentials are constructed in test code; ensure CI secrets are always present and avoid logging sensitive responses or errors that might include auth details.

const orgId = process.env["ORGNAME"];
const streamName = "e2e_automate";
const basicAuthCredentials = Buffer.from(
  `${process.env["ZO_ROOT_USER_EMAIL"]}:${process.env["ZO_ROOT_USER_PASSWORD"]}`
).toString('base64');

const headers = {
  "Authorization": `Basic ${basicAuthCredentials}`,
  "Content-Type": "application/json",
};

try {
  const response = await page.evaluate(async ({ url, headers, orgId, streamName, logsdata }) => {
    const fetchResponse = await fetch(`${url}/api/${orgId}/${streamName}/_json`, {
      method: 'POST',
      headers: headers,
      body: JSON.stringify(logsdata)
    });
    if (!fetchResponse.ok) {
      throw new Error(`HTTP error! status: ${fetchResponse.status}`);
    }
    return await fetchResponse.json();
  }, {
    url: process.env.INGESTION_URL,
    headers: headers,
    orgId: orgId,
    streamName: streamName,
    logsdata: logsdata
  });
  console.log('Ingestion response:', response);
  return response;
} catch (error) {
  console.error('Ingestion failed:', error);
  throw error;
}
Persistence Assumption

The persistence test reloads the page without re-applying filters or ensuring the same stream context; validate that the stream and time range are still set after reload before asserting header presence.

  // Refresh the page
  await page.reload();
  await page.waitForLoadState('domcontentloaded');
  await page.waitForTimeout(2000);
  testLogger.info('Page refreshed');

  // Verify field is still visible in table after refresh
  await pageManager.logsPage.expectFieldInTableHeader(fieldName);
  testLogger.info('Field persistence after page refresh verified successfully');
});

@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2025

PR Code Suggestions ✨

Latest suggestions up to 7dc3f92
Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Verify header is truly removed

Assert the actual absence of the removed field header to avoid false positives when
'source' happens to be visible for unrelated reasons. First check that the field
header does not exist before falling back to any secondary expectations.

tests/ui-testing/pages/logsPages/logsPage.js [1963-1966]

 async expectFieldNotInTableHeader(fieldName) {
-    // When field is removed, the source column should be visible again
-    return await expect(this.page.locator('[data-test="log-search-result-table-th-source"]').getByText('source')).toBeVisible();
+    await expect(this.page.locator(`[data-test="log-search-result-table-th-${fieldName}"]`)).toHaveCount(0);
+    await expect(this.page.locator('[data-test="log-search-result-table-th-source"]').getByText('source')).toBeVisible();
 }
Suggestion importance[1-10]: 8

__

Why: Asserting the absence of [data-test="log-search-result-table-th-${fieldName}"] directly improves test correctness by preventing false positives; the proposed code aligns with the context.

Medium
Possible issue
Validate required environment variables

Guard against missing credentials to avoid encoding 'undefined:undefined' and
sending invalid Authorization headers. Fail fast with a clear error when required
environment variables are absent.

tests/ui-testing/playwright-tests/Logs/logstable.spec.js [12-14]

-const basicAuthCredentials = Buffer.from(
-  `${process.env["ZO_ROOT_USER_EMAIL"]}:${process.env["ZO_ROOT_USER_PASSWORD"]}`
-).toString('base64');
+const user = process.env["ZO_ROOT_USER_EMAIL"];
+const pass = process.env["ZO_ROOT_USER_PASSWORD"];
+if (!user || !pass) {
+  throw new Error("Missing ingestion credentials: ZO_ROOT_USER_EMAIL or ZO_ROOT_USER_PASSWORD");
+}
+const basicAuthCredentials = Buffer.from(`${user}:${pass}`).toString('base64');
Suggestion importance[1-10]: 7

__

Why: Guarding against missing ZO_ROOT_USER_EMAIL/ZO_ROOT_USER_PASSWORD prevents sending invalid Authorization headers; the change is accurate and low-risk but not critical.

Medium
Ensure ingestion URL is set

Validate that process.env.INGESTION_URL is defined before passing it into the
browser context; otherwise the evaluated code will fetch 'undefined/api/...'. Throw
a descriptive error when the URL is missing.

tests/ui-testing/playwright-tests/Logs/logstable.spec.js [33-38]

+const ingestionUrl = process.env.INGESTION_URL;
+if (!ingestionUrl) {
+  throw new Error("Missing INGESTION_URL for ingestion");
+}
 const response = await page.evaluate(async ({ url, headers, orgId, streamName, logsdata }) => {
   const fetchResponse = await fetch(`${url}/api/${orgId}/${streamName}/_json`, {
     method: 'POST',
-    headers: headers,
+    headers,
     body: JSON.stringify(logsdata)
   });
   if (!fetchResponse.ok) {
     throw new Error(`HTTP error! status: ${fetchResponse.status}`);
   }
   return await fetchResponse.json();
 }, {
-  url: process.env.INGESTION_URL,
-  headers: headers,
-  orgId: orgId,
-  streamName: streamName,
-  logsdata: logsdata
+  url: ingestionUrl,
+  headers,
+  orgId,
+  streamName,
+  logsdata
 });
Suggestion importance[1-10]: 7

__

Why: Verifying INGESTION_URL avoids fetching from an undefined URL; it's a correct, pragmatic check improving test reliability.

Medium

Previous suggestions

Suggestions up to commit 7dc3f92
CategorySuggestion                                                                                                                                    Impact
Possible issue
Replace timeout with state wait

Replace fixed timeouts with assertions that wait for the UI state change to complete
to avoid flakiness. After clicking, wait for the table header for the given field to
become visible.

tests/ui-testing/pages/logsPages/logsPage.js [1949-1952]

 async clickAddFieldToTableButton(fieldName) {
     await this.page.locator(`[data-test="log-search-index-list-add-${fieldName}-field-btn"]`).click();
-    await this.page.waitForTimeout(1000);
+    await expect(this.page.locator(`[data-test="log-search-result-table-th-${fieldName}"]`)).toBeVisible();
 }
Suggestion importance[1-10]: 7

__

Why: Replacing a fixed timeout with an assertion-based wait reduces flakiness and aligns with Playwright best practices; the change is accurate and contextually correct.

Medium
Wait for field to disappear

Avoid fixed delays after removal; instead, wait for the field to disappear to ensure
reliable verification. Use an assertion that the target header is hidden/not
present.

tests/ui-testing/pages/logsPages/logsPage.js [1954-1957]

 async clickRemoveFieldFromTableButton(fieldName) {
     await this.page.locator(`[data-test="log-search-index-list-remove-${fieldName}-field-btn"]`).click();
-    await this.page.waitForTimeout(1000);
+    await expect(this.page.locator(`[data-test="log-search-result-table-th-${fieldName}"]`)).toBeHidden();
 }
Suggestion importance[1-10]: 7

__

Why: Waiting for the specific header to be hidden is more reliable than a static delay; it's a sound, targeted improvement with moderate impact.

Medium
General
Assert field absence directly

Verify the absence of the specific field instead of relying on the 'source' column
as a proxy, which can cause false positives. Assert that the removed field header is
not visible or does not exist.

tests/ui-testing/pages/logsPages/logsPage.js [1963-1966]

 async expectFieldNotInTableHeader(fieldName) {
-    // When field is removed, the source column should be visible again
-    return await expect(this.page.locator('[data-test="log-search-result-table-th-source"]').getByText('source')).toBeVisible();
+    await expect(this.page.locator(`[data-test="log-search-result-table-th-${fieldName}"]`)).toHaveCount(0);
 }
Suggestion importance[1-10]: 7

__

Why: Directly asserting the removed field's absence avoids proxy checks that can be flaky; the improved code matches the intent and enhances test robustness.

Medium

@neha00290 neha00290 marked this pull request as ready for review September 2, 2025 12:13
@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2025

Persistent review updated to latest commit 7dc3f92

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Summary

This PR adds end-to-end testing capabilities for logs table field management functionality. The changes extend the existing LogsPage Page Object Model (POM) with new methods to support adding and removing fields from the logs table view, and creates a comprehensive test suite to verify this functionality.

Key Changes:

  1. LogsPage POM Extension - Added five new methods to tests/ui-testing/pages/logsPages/logsPage.js:

    • hoverOnFieldExpandButton() - Reveals field action buttons by hovering
    • clickAddFieldToTableButton() - Adds a field to the table view
    • clickRemoveFieldFromTableButton() - Removes a field from the table view
    • expectFieldInTableHeader() - Verifies field presence in table headers
    • expectFieldNotInTableHeader() - Verifies field removal from table headers
  2. New Test Suite - Created tests/ui-testing/playwright-tests/Logs/logstable.spec.js with comprehensive E2E tests that:

    • Test the complete workflow of adding and removing fields
    • Verify field persistence after page refresh
    • Include data ingestion helpers to seed test data
    • Handle proper test setup/teardown with navigation and mode toggling
  3. CI Integration - Updated .github/workflows/playwright.yml to include the new test file in the Logs-Core test category, ensuring it runs as part of the continuous integration pipeline.

The implementation follows established testing patterns in the codebase, using data-test attributes for stable element selection and including appropriate wait timeouts to handle asynchronous UI updates. The new methods integrate seamlessly with the existing LogsPage class architecture and maintain consistent naming conventions.

Confidence score: 5/5

  • This PR is extremely safe to merge with minimal risk of production issues
  • Score reflects simple test-only changes with no impact on production code
  • No files require special attention as these are isolated testing improvements

2 files reviewed, 1 comment

Edit Code Review Bot Settings | Greptile

@nikhilsaikethe nikhilsaikethe merged commit 24fca0d into main Sep 2, 2025
29 checks passed
@nikhilsaikethe nikhilsaikethe deleted the logs-add-to-table branch September 2, 2025 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants