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

[ENHANCEMENT] Refactor feature chart methods to be more generic and reusable #964

Open
DanielRyanSmith opened this issue Dec 10, 2024 · 1 comment
Assignees
Labels
enhancement New feature or request

Comments

@DanielRyanSmith
Copy link
Collaborator

Per @jcscottiii's comments on #944:

We should leverage generics since this is similar to createFeatureSupportDataFromMap.

A main generic method like:

createDataFromMap<T>(
  data: Map<string, T[]>,
  browsers: string[],
  valueExtractor: (row: T) => number,
  tooltipGenerator: (row: T, browser: string) => string,
  totalLabel?: string,
): WebStatusDataObj {
const dateToTotalMap = new Map<number, number>();
  for (const row of browserData) {
        for (const row of browserData) {
    ...
       if (totalLabel) {
            const total = Math.max(
              dateToTotalMap.get(roundedTimestamp) || 0,
              (row as any).total_tests_count || 0
            );
            dateToTotalMap.set(roundedTimestamp, total);
       }
     }
  ...
  }
  ....
  
    if (totalLabel) {
      dataObj.cols.push({
        type: 'number',
        label: totalLabel,
        role: 'data',
      });
    }
  ...
  }
  ...
  for (const datum of data) {
    if (totalLabel) {
      rowData.push(dateToTotalMap.get(dateMs)!);
    }
  }
}

Then each method could be simplified to something like this:

// For feature support data
createFeatureSupportDataFromMap(): WebStatusDataObj {
  return  this.createDataFromMap(
    this.featureSupport,
    this.featureSupportBrowsers,
    row => row.test_pass_count!,
    (row, browser) => `${BROWSER_ID_TO_LABEL[browser]}: ${row.test_pass_count} of ${row.total_tests_count}`,
    'Total Number of Tests'
  );
}

// For feature usage data
createFeatureUsageDataFromMap(): WebStatusDataObj{
  return this.createDataFromMap(
    this.featureUsage,
    this.featureUsageBrowsers,
    row => row.usage ? row.usage * 100 : 0,
    (row, browser) => `${BROWSER_ID_TO_LABEL[browser]}: ${row.usage ? row.usage * 100 : 0}%`
  );
}

Also, could you add some unit tests for these two methods during that refactor PR please?

One test should create a component with test support data. Another with test usage data. Each test should call the method and assert things like:

    // For support test
    // Assertions for columns
    expect(result.cols.length).to.equal(6); // Expecting 6 columns (date + 2 for each browser + total)
    expect(result.cols[5].label).to.equal('Total Tests'); 

    // Assertions for rows
    expect(result.rows.length).to.equal(N); // Up to you on how many rows you 

    // Check data for the first row (rounded timestamp: 2024-12-08T12:00:00.000Z)
    const firstRow = result.rows[0];
    expect(firstRow[0]).to.be.instanceOf(Date);
    expect(firstRow[1]).to.equal(50); // Chrome pass count
    expect(firstRow[2]).to.equal('Chrome: 50 of 100'); // Chrome tooltip
    expect(firstRow[3]).to.equal(45); // Firefox pass count
    expect(firstRow[4]).to.equal('Firefox: 45 of 100'); // Firefox tooltip
    expect(firstRow[5]).to.equal(100); // Total tests count
    // for usage test
    // Assertions for columns
    expect(result.cols.length).to.equal(3); // Expecting 3 columns (date + 2 for each browser)

    // Assertions for rows
   expect(result.rows.length).to.equal(N); // Up to you on how many rows you 

  // Check data for the first row
  const firstRow = result.rows[0];
  expect(firstRow[0]).to.be.instanceOf(Date);
  expect(firstRow[1]).to.equal(50); // Chrome usage
  expect(firstRow[2]).to.equal('Chrome: 50%'); // Chrome tooltip

A similar refactoring + unit tests should be done for generateFeatureUsageChartOptions and generateFeatureSupportChartOptions

I think the method signature could be something like:

generateChartOptions(
  selectedBrowsers: string[],
  vAxisTitle: string
): google.visualization.ComboChartOptions {
....
}

_startUsageMetricsTask and _startMetricsTask could be refactored.

something like this:

type LoadingTaskType = '_loadingMetricsTask' | '_loadingUsageTask';
private async _startMetricsFetchingTask<T extends LoadingTaskType>(
  manualRun: boolean,
  dataFetcher: (apiClient: any, featureId: string, startDate: Date, endDate: Date) => Promise<void>,
  taskType: T,
) {
  this[taskType]?.abort(); // Access the task property using bracket notation

  this[taskType] = new Task(this, { // Assign the new task to the correct property
    args: () => [this.apiClient, this.featureId],
    task: async ([apiClient, featureId]) => {
      if (typeof apiClient === 'object' && typeof featureId === 'string') {
        await dataFetcher(apiClient, featureId, this.startDate, this.endDate);
      }
    },
  });

  if (manualRun) {
    this[taskType]!.autoRun = false; // Non-null assertion is safe here
    await this[taskType]!.run();
  }
}

// Example calls
// Manual run of support data
await this._startDataFetchingTask(true, this._fetchFeatureSupportData, '_loadingMetricsTask');
// Non manual run of usage data
await this._startDataFetchingTask(false, this._fetchFeatureUsageData, '_loadingUsageTask');
@KyleJu
Copy link
Collaborator

KyleJu commented Dec 16, 2024

The same applies to the createDisplayDataFromMap in webstatus-stats-page.ts

@DanielRyanSmith DanielRyanSmith self-assigned this Dec 22, 2024
@DanielRyanSmith DanielRyanSmith moved this to In Progress in webstatus.dev Dec 22, 2024
jcscottiii added a commit that referenced this issue Dec 27, 2024
**Background:**

When backfilling usage data, the feature usage chart displayed only a few data points, not the complete set.  Network inspection revealed that multiple pages of data were successfully retrieved.

**Repro:**

1. Navigate to: https://website-webstatus-dev.corp.goog/features/flexbox?showUsageChart

**Solution:**

This change aligns the feature usage chart pagination with the feature support chart's implementation: https://github.com/GoogleChrome/webstatus.dev/blob/b84a9ac02c2455dd6e5043615fc2a084f6980a46/frontend/src/static/js/components/webstatus-feature-page.ts#L621-L650

**Testing:**

Comprehensive unit tests will be included with the refactor outlined in #964.

To validate the pagination logic, this commit increases the number of data points displayed to over 100, enabling testing through Playwright. To optimize Playwright test performance, the initial data load specifically filters for the feature used in the feature page test, mitigating delays associated with loading excessive fake data.
github-merge-queue bot pushed a commit that referenced this issue Dec 30, 2024
**Background:**

When backfilling usage data, the feature usage chart displayed only a few data points, not the complete set.  Network inspection revealed that multiple pages of data were successfully retrieved.

**Repro:**

1. Navigate to: https://website-webstatus-dev.corp.goog/features/flexbox?showUsageChart

**Solution:**

This change aligns the feature usage chart pagination with the feature support chart's implementation: https://github.com/GoogleChrome/webstatus.dev/blob/b84a9ac02c2455dd6e5043615fc2a084f6980a46/frontend/src/static/js/components/webstatus-feature-page.ts#L621-L650

**Testing:**

Comprehensive unit tests will be included with the refactor outlined in #964.

To validate the pagination logic, this commit increases the number of data points displayed to over 100, enabling testing through Playwright. To optimize Playwright test performance, the initial data load specifically filters for the feature used in the feature page test, mitigating delays associated with loading excessive fake data.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: In Progress
Development

No branches or pull requests

2 participants