How did we get this number?

This number inherits from existing logic (from GO backend more earlier). Up to change if necessary.

Should this be a module or class level constant?

It is only used in function level.

This looks like a constant, how is this different from the one you defined in cocoon config?

Having the same value defined in several places makes an update of constant error prone.

can we use a better name? from the name we don't know what it refers to.

Replace with defaultBranch, and move it to cocoon config.

Use config.maxRecords everywhere, you have three variables pointing to the same value.

Updated.

rm and use the config.maxRecords.

Replaced maxRecords with config.maxRecords. But we need to keep variable masterLimit, which is used to track the number of master commits.

This variable name is really confusing I'd recommend using a different name for it. In this case is not clear if you want 'master' instead.

changed to numberOfMasterCommits to make it clear.

please add docs for the parameters.

Added.

1 hour is too much, how did we get to this value?

Like already mentioned in the comment doc: values are inserted into datastore later than commit. The workflow inserts commit first into datastore, suppose with timestamp1, and then inserts values of different tests results later once tests finish, suppose with timestampX. timestampX varies for different tests/commits, depending on test execution time and environment. Based on our infra metrics collected/analyzed, executing time of a commit row is less than 1 hour, that's why I put the 1 hour timeframe. If too small, we may miss some values.

Adding, subtracting time can cause race conditions(we don't control the time) should we fix the query to return only commits that have all the data instead?

I guess the current logic is designed like this to reduce datastore traffic. If we query timeseriesValue based on commit, then we need to make config.maxRecords requests each time the web refreshes.

With current logic, it needs to query datastore only once.

Also, I don't think race condition is an issue here. This API reads data all the time, rather than writes anything. If data is not there yet, it simply show empty value on the web.

My suggestion was to update the query to return only commits with complete results up to the passed timestamp. That should not affect the current performance and will eliminate the need to guess if a commit is complete based on estimates of the execution time.

Unluckily that is not possible due to datastore query limitation.
To achieve what you suggested, we need to add a commit list which contains all commits to filter out our interested records of timeseriesValue. However, dart db supports only simple filter options like = (https://github.com/dart-lang/gcloud/blob/master/lib/src/db/db.dart#L128).

Added a TODO: flutter/flutter#56731

Should we place this class in models?

This class is used to hold temporary results, without a general-wide usage purpose across the whole app. We may just keep it here loally.

In that case please document its purpose.

Added.

Please add a constant for this value it will make easier to change in a single place.

Added.

Do we need to add tests that are adding records from master and one release branch?

reports statuses with input branch is testing this case.

In that case can we refactor the tests to cover more atomic cases? Having tests that are testing too much logic require to understand the entire workflow to fix them when they break because we don't know exactly what is failing.

could you please elaborate on what you mean by covering atomic cases. Do you mean test their sub functions individually? In that case, we may need to move those private functions public.

BTW: Each API in cocoon performs a specific purpose. It seems that all APIs' tests test their whole logic no matter the API is as complex as https://github.com/flutter/cocoon/blob/master/app_dart/lib/src/request_handlers/check_for_waiting_pull_requests.dart, or as simply as https://github.com/flutter/cocoon/blob/master/app_dart/lib/src/request_handlers/get_branches.dart.

To make the test more easily readable, I have added some documents.

Sent the link to tests best practices by email, please read it and refactor accordingly.

Thanks for the link! Have added more atomic tests.

Here and below, let's try to keep test names short. If we need to add full descriptions let's add it as docs.

Use shorter test names, and put extra words to doc.