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

feat: add performance assessment to acceptance tests #1771

Merged
merged 23 commits into from
May 31, 2024
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 55 additions & 33 deletions .github/workflows/acceptance_test.yml
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
name: Rule acceptance tests

on:
pull_request:
branches: [ master ]
Expand All @@ -16,16 +15,24 @@ on:
- 'web/**'
- '.github/workflows/web_**.yml'
- '.github/workflows/stg_web_**.yml'
types: [ opened, synchronize, reopened, ready_for_review ]
concurrency:
group: ${{ github.head_ref }}
cancel-in-progress: true
jobs:
fail_if_pull_request_is_draft: # Fails in order to indicate that pull request needs to be marked as ready to review to pass.
if: github.event.pull_request.draft == true
runs-on: ubuntu-latest
steps:
- name: Fail if PR is a draft
run: exit 1
pre_ci:
name: Prepare CI environment
if: github.event.pull_request.draft == false # Skip this job and its dependencies if the PR is draft
runs-on: ubuntu-latest
steps:
- name: Checkout Project
uses: actions/checkout@v3
uses: actions/checkout@v4
with:
# We need to fetch with a depth of 2 for pull_request so we can do HEAD^2
fetch-depth: 2
Expand All @@ -48,62 +55,62 @@ jobs:
needs: pre_ci
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- uses: gradle/wrapper-validation-action@v1
- uses: actions/checkout@v4
- uses: gradle/actions/wrapper-validation@v3
pack-snapshot:
needs: [ validate-gradle-wrapper ]
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- uses: actions/checkout@v4
- name: Set up JDK 11
uses: actions/setup-java@v3
uses: actions/setup-java@v4
with:
java-version: '11'
distribution: 'temurin'
- name: Cache Gradle packages
uses: actions/cache@v3
uses: actions/cache@v4
with:
path: ~/.gradle/caches
key: ${{ runner.os }}-gradle-${{ hashFiles('**/*.gradle') }}
restore-keys: ${{ runner.os }}-gradle
- name: Set up Gradle
uses: gradle/actions/setup-gradle@v3
- name: Package cli app jar with Gradle
uses: gradle/gradle-build-action@v2
with:
arguments: shadowJar
run: ./gradlew shadowJar
- name: Persist gtfs-validator snapshot jar
uses: actions/upload-artifact@v3
uses: actions/upload-artifact@v4
with:
name: gtfs-validator-snapshot
path: cli/build/libs/gtfs-validator-*-cli.jar
- name: Persist comparator snapshot jar
uses: actions/upload-artifact@v3
uses: actions/upload-artifact@v4
with:
name: comparator-snapshot
path: output-comparator/build/libs/output-comparator-*-cli.jar
pack-master:
needs: [ validate-gradle-wrapper ]
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- uses: actions/checkout@v4
with:
ref: master
- name: Set up JDK 11
uses: actions/setup-java@v3
uses: actions/setup-java@v4
with:
java-version: '11'
distribution: 'temurin'
- name: Cache Gradle packages
uses: actions/cache@v3
uses: actions/cache@v4
with:
path: ~/.gradle/caches
key: ${{ runner.os }}-gradle-${{ hashFiles('**/*.gradle') }}
restore-keys: ${{ runner.os }}-gradle
- name: Set up Gradle
uses: gradle/actions/setup-gradle@v3
- name: Package cli app jar with Gradle
uses: gradle/gradle-build-action@v2
with:
arguments: shadowJar
run: ./gradlew shadowJar
- name: Persist gtfs-validator jar from master branch
uses: actions/upload-artifact@v3
uses: actions/upload-artifact@v4
with:
name: gtfs-validator-master
path: cli/build/libs/gtfs-validator-*-cli.jar
Expand All @@ -113,7 +120,7 @@ jobs:
runs-on: ubuntu-latest
steps:
- name: Checkout repository code
uses: actions/checkout@v3
uses: actions/checkout@v4
- name: Install dependencies
run: |
pip install -r scripts/mobility-database-harvester/requirements.txt
Expand All @@ -125,7 +132,7 @@ jobs:
echo "matrix=$DATASETS" >> $GITHUB_OUTPUT
- name: Persist metadata
if: always()
uses: actions/upload-artifact@v3
uses: actions/upload-artifact@v4
with:
name: datasets_metadata
path: scripts/mobility-database-harvester/datasets_metadata
Expand All @@ -139,43 +146,58 @@ jobs:
strategy:
matrix: ${{ fromJson(needs.fetch-urls.outputs.matrix) }}
steps:
- uses: actions/checkout@v3
- uses: actions/checkout@v4
- name: Download .jar file from master branch
uses: actions/download-artifact@v3
uses: actions/download-artifact@v4
with:
name: gtfs-validator-master
path: gtfs-validator-master
- name: Download latest changes .jar file from previous job
uses: actions/download-artifact@v3
uses: actions/download-artifact@v4
with:
name: gtfs-validator-snapshot
path: gtfs-validator-snapshot
- name: Extract and concatenate IDs
run: |
concatenated_ids=$(bash ./scripts/extract_ids.sh '${{ matrix.data }}')
echo "CONCATENATED_IDS=$concatenated_ids" >> $GITHUB_ENV
echo "CONCATENATED_IDS=$concatenated_ids"
- name: Run validators on queued URLs
run: |
queue="${{ matrix.data }}"
bash ./scripts/queue_runner.sh --include-master $queue
env:
OUTPUT_BASE: ${{ github.sha }}
- name: Persist reports
uses: actions/upload-artifact@v3
uses: actions/upload-artifact@v4
with:
name: reports_all
name: reports_${{ env.CONCATENATED_IDS }}
path: ${{ github.sha }}/output
merge-reports-artifacts:
runs-on: ubuntu-latest
needs: [ get-reports ]
steps:
- name: Merge Artifacts
uses: actions/upload-artifact/merge@v4
with:
name: reports_all
pattern: reports_*
delete-merged: true
compare-outputs:
needs: [ get-reports ]
needs: [ merge-reports-artifacts ]
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- uses: actions/checkout@v4
- name: Download comparator .jar file from previous job
uses: actions/download-artifact@v3
uses: actions/download-artifact@v4
with:
name: comparator-snapshot
- name: Retrieve reports from previous job
uses: actions/download-artifact@v3
uses: actions/download-artifact@v4
with:
name: reports_all
- name: Retrieve gtfs latest versions from previous job
uses: actions/download-artifact@v3
uses: actions/download-artifact@v4
with:
name: datasets_metadata
- name: Generate acceptance report test
Expand All @@ -193,15 +215,15 @@ jobs:
--run_id ${{github.run_id}}
- name: Persist acceptance test reports
if: always()
uses: actions/upload-artifact@v3
uses: actions/upload-artifact@v4
with:
name: acceptance_test_report
path: acceptance-test-output
- name: Generate PR comment
id: generate-comment
if: always()
run: |
PR_COMMENT=$(< acceptance-test-output/acceptance_report_summary.txt)
PR_COMMENT=$(< acceptance-test-output/acceptance_report_summary.md)
echo "PR_COMMENT<<EOF" >> $GITHUB_ENV
echo "$PR_COMMENT" >> $GITHUB_ENV
echo "EOF" >> $GITHUB_ENV
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,19 +42,28 @@
public class ValidationReportDeserializer implements JsonDeserializer<ValidationReport> {

private static final String NOTICES_MEMBER_NAME = "notices";
private static final String SUMMARY_MEMBER_NAME = "summary";
private static final String VALIDATION_TIME_MEMBER_NAME = "validationTimeSeconds";

@Override
public ValidationReport deserialize(
JsonElement json, Type typoOfT, JsonDeserializationContext context) {
Set<NoticeReport> notices = new LinkedHashSet<>();
JsonObject rootObject = json.getAsJsonObject();
// Note that the json file contains the summary in addition to the notices, but it is ignored
// since currently the report comparison is only on the notices
// since currently the report comparison is only on the notices and the validation time.
Double validationTimeSeconds = null;
if (rootObject.has(SUMMARY_MEMBER_NAME)) {
JsonObject summaryObject = rootObject.getAsJsonObject(SUMMARY_MEMBER_NAME);
if (summaryObject.has(VALIDATION_TIME_MEMBER_NAME)) {
validationTimeSeconds = summaryObject.get(VALIDATION_TIME_MEMBER_NAME).getAsDouble();
}
}
JsonArray noticesArray = rootObject.getAsJsonArray(NOTICES_MEMBER_NAME);
for (JsonElement childObject : noticesArray) {
notices.add(Notice.GSON.fromJson(childObject, NoticeReport.class));
}
return new ValidationReport(notices);
return new ValidationReport(notices, validationTimeSeconds);
}

public static <T extends Notice> JsonObject serialize(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ public class ValidationReport {
.serializeSpecialFloatingPointValues()
.create();
private final Set<NoticeReport> notices;
private final Double validationTimeSeconds;

/**
* Public constructor needed for deserialization by {@code ValidationReportDeserializer}. Only
Expand All @@ -49,7 +50,19 @@ public class ValidationReport {
* @param noticeReports set of {@code NoticeReport}s
*/
public ValidationReport(Set<NoticeReport> noticeReports) {
this(noticeReports, null);
}

/**
* Public constructor needed for deserialization by {@code ValidationReportDeserializer}. Only
* stores information for error {@code NoticeReport}.
*
* @param noticeReports set of {@code NoticeReport}s
* @param validationTimeSeconds the time taken to validate the GTFS dataset
*/
public ValidationReport(Set<NoticeReport> noticeReports, Double validationTimeSeconds) {
this.notices = Collections.unmodifiableSet(noticeReports);
this.validationTimeSeconds = validationTimeSeconds;
}

/**
Expand All @@ -69,6 +82,10 @@ public Set<NoticeReport> getNotices() {
return notices;
}

public Double getValidationTimeSeconds() {
return validationTimeSeconds;
}

/**
* Determines if two validation reports are equal regardless of the order of the fields in the set
* of {@code NoticeReport}.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ public class JsonReportSummary {
private JsonReportFeedInfo feedInfo;
private List<JsonReportAgencyMetadata> agencies;
private Set<String> files;
private Double validationTimeSeconds;

@SerializedName("counts")
private JsonReportCounts jsonReportCounts;
Expand Down Expand Up @@ -65,6 +66,7 @@ public JsonReportSummary(
if (feedMetadata != null) {
if (feedMetadata.feedInfo != null) {
this.feedInfo = new JsonReportFeedInfo(feedMetadata.feedInfo);
this.validationTimeSeconds = feedMetadata.validationTimeSeconds;
} else {
logger.atSevere().log(
"No feed info for feed "
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ public class FeedMetadata {
public ArrayList<AgencyMetadata> agencies = new ArrayList<>();
private ImmutableSortedSet<String> filenames;

public double validationTimeSeconds;

// List of features that only require checking the presence of one record in the file.
private final List<Pair<String, String>> FILE_BASED_FEATURES =
List.of(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,21 +134,22 @@ public Status run(ValidationRunnerConfig config) {
}
FeedMetadata feedMetadata = FeedMetadata.from(feedContainer, gtfsInput.getFilenames());
closeGtfsInput(gtfsInput, noticeContainer);
feedMetadata.validationTimeSeconds = (System.nanoTime() - startNanos) / 1e9;

// Output
exportReport(feedMetadata, noticeContainer, config, versionInfo);
printSummary(startNanos, feedContainer, feedLoader, anyTableLoader);
printSummary(feedMetadata, feedContainer, feedLoader, anyTableLoader);
return Status.SUCCESS;
}

/**
* Prints validation metadata.
*
* @param startNanos start time as nanoseconds
* @param feedMetadata the {@code FeedMetadata}
* @param feedContainer the {@code GtfsFeedContainer}
*/
public static void printSummary(
long startNanos,
FeedMetadata feedMetadata,
GtfsFeedContainer feedContainer,
GtfsFeedLoader loader,
AnyTableLoader anyTableLoader) {
Expand Down Expand Up @@ -198,7 +199,7 @@ public static void printSummary(

logger.atSevere().log(b.toString());
}
logger.atInfo().log("Validation took %.3f seconds%n", (endNanos - startNanos) / 1e9);
logger.atInfo().log("Validation took %.3f seconds%n", feedMetadata.validationTimeSeconds);
logger.atInfo().log(feedContainer.tableTotalsText());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ private static FeedMetadata generateFeedMetaData() {
3 // Should not be present in the resulting GSON
);
feedMetadata.specFeatures = Map.of("Feature1", false, "Feature2", true);
feedMetadata.validationTimeSeconds = 100.0;
return feedMetadata;
}

Expand Down Expand Up @@ -118,6 +119,7 @@ public void withFeedMetadataWithConfigTest() throws Exception {
+ "\"countryCode\":\"GB\","
+ "\"dateForValidation\":\"2020-01-02\","
+ "\"feedInfo\":{\"publisherName\":\"value1\",\"publisherUrl\":\"value2\"},"
+ "\"validationTimeSeconds\":100.0,"
+ "\"agencies\":["
+ "{\"name\":\"agency1\",\"url\":\"some URL 1\",\"phone\":\"phone1\",\"email\":\"email1\"},"
+ "{\"name\":\"agency1\",\"url\":\"some URL 1\",\"phone\":\"phone1\",\"email\":\"email1\"}],"
Expand All @@ -139,6 +141,7 @@ public void withFeedMetadataNoConfigTest() throws Exception {
+ "\"validatedAt\":\"now\","
+ "\"threads\":0,"
+ "\"feedInfo\":{\"publisherName\":\"value1\",\"publisherUrl\":\"value2\"},"
+ "\"validationTimeSeconds\":100.0,"
+ "\"agencies\":["
+ "{\"name\":\"agency1\",\"url\":\"some URL 1\",\"phone\":\"phone1\",\"email\":\"email1\"},"
+ "{\"name\":\"agency1\",\"url\":\"some URL 1\",\"phone\":\"phone1\",\"email\":\"email1\"}],"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,15 @@ public class Main {

private static final FluentLogger logger = FluentLogger.forEnclosingClass();
static final String ACCEPTANCE_REPORT_JSON = "acceptance_report.json";
static final String ACCEPTANCE_REPORT_SUMMARY_TXT = "acceptance_report_summary.txt";
static final String ACCEPTANCE_REPORT_SUMMARY_MD = "acceptance_report_summary.md";
private static final int IO_EXCEPTION_EXIT_CODE = 1;
private static final int COMPARISON_FAILURE_EXIT_CODE = 2;
private static final Gson GSON =
new GsonBuilder().serializeNulls().disableHtmlEscaping().create();
new GsonBuilder()
.serializeNulls()
.disableHtmlEscaping()
.serializeSpecialFloatingPointValues()
.create();

public static void main(String[] argv) {
int rc = run(argv);
Expand Down Expand Up @@ -122,7 +126,7 @@ private static void exportAcceptanceReport(AcceptanceReport report, String outpu
private static void exportReportSummary(String reportSummary, String outputBase) {
try {
Files.write(
Paths.get(outputBase, ACCEPTANCE_REPORT_SUMMARY_TXT),
Paths.get(outputBase, ACCEPTANCE_REPORT_SUMMARY_MD),
reportSummary.getBytes(StandardCharsets.UTF_8));
} catch (IOException e) {
logger.atSevere().withCause(e).log("Cannot store acceptance test report summary file");
Expand Down
Loading
Loading