feat: acceptance tests #848

lionel-nj · 2021-04-09T19:52:50Z

closes #734

Summary:

This PR provides changes to create a new module to be used for acceptance tests

Expected behavior:

Input data:

/reports/archive-id-1
  - latest.json
  - reference.json
/reports/archive-id-2
  - latest.json
  - reference.json
/reports/archive-id-3
  - latest.json
  - reference.json

where:

latest.json is the validation report produced by the snapshot version of the validation
reference.json is the validation report generated by the version of reference of the validator (typically the version published on the master branch)

Comparison process

Validation reports (latest.json and reference.json) are compared for each dataset-id-value.
If latest.json contains a type of error notice (identified by notice_code) that is not included in reference.json, then it is flagged by incrementing a counter related to the dataset in question (identified by an id).
If the value in this counter is greater than the allowed threshold (determined by command line input, please see documentation in /docs/ACCEPTANCE_TEST.md), then the dataset is flagged as faulty.

At the end the percentage of new faulty datasets is compared to the allowed threshold (determined by command line input, please see documentation in /docs/ACCEPTANCE_TEST.md) to determine if a rule is "acceptable" or not.

Final output

The final outputs:

A json file named acceptance_report.json that contains information about the difference encountered for each source, formatted as follows:

{
  "newErrors": [
    {
      "noticeCode": "first_notice_code",
      "affectedSourcesCount": 2,
      "affectedSources": [
        {
          "sourceId": "source-id-1",
          "sourceUrl": "url to the latest version of the dataset issued by source-id-1",
          "count": 4
        },
        {
          "sourceId": "source-id-2",
          "sourceUrl": "url to the latest version of the dataset issued by source-id-2",
          "count": 6
        }
      ]
    },
    {
      "noticeCode": "fourth_notice_code",
      "affectedSourcesCount": 1,
      "affectedSources": [
        {
          "sourceId": "source-id-5",
          "sourceUrl": "url to the latest version of the dataset issued by source-id-5",
          "count": 5
        }
      ]
    },
    {
      "noticeCode": "second_notice_code",
      "affectedSourcesCount": 1,
      "affectedSources": [
        {
          "sourceId": "source-id-2",
          "sourceUrl": "url to the latest version of the dataset issued by source-id-2",
          "count": 40
        }
      ]
    },
    {
      "noticeCode": "third_notice_code",
      "affectedSourcesCount": 3,
      "affectedSources": [
        {
          "sourceId": "source-id-1",
          "sourceUrl": "url to the latest version of the dataset issued by source-id-1",
          "count": 40
        },
        {
          "sourceId": "source-id-3",
          "sourceUrl": "url to the latest version of the dataset issued by source-id-3",
          "count": 15
        },
        {
          "sourceId": "source-id-5",
          "sourceUrl": "url to the latest version of the dataset issued by source-id-5",
          "count": 2
        }
      ]
    }
  ]
}

A json file named corrupted_sources_report.json that contains information about soruces that could not be taken into account for the acceptance test; which is formatted as follows:

{
  "corruptedSources": [
    "source-id-1",
    "source-id-2",
  ],
  "sourceIdCount": 1245,
  "status": "valid",
  "corruptedSourcesCount": 2,
  "maxPercentageCorruptedSources": 2
}

A console log that underlines the percentage of existing dataset that contains more than 1 new type of error.
A comment on the PR with link to the acceptance test report.
Workflow status is green ( ✅ ) is acceptance test passed or red ( ❌ ) if it did not.

Error handling

Directory output is empty
log: "Specified directory is empty, cannot generate acceptance tests report."
No report is available for a given id
System exits on error code 1
One of the reports is not available for a given id
System exits on error code 1

Please make sure these boxes are checked before submitting your pull request - thanks!

Run the unit tests with gradle test to make sure you didn't break anything
Format the title like "feat: [new feature short description]". Title must follow the Conventional Commit Specification(https://www.conventionalcommits.org/en/v1.0.0/).
Linked all relevant issues
Include screenshot(s) showing how this pull request works and fixes the issue(s)

integration/src/main/java/org/mobilitydata/gtfsvalidator/integration/Main.java

lionel-nj · 2021-04-14T00:25:30Z

After 1 billion trials, this works: 7dc7016. Stoping here for today, resuming tomorrow.

To all those that received a bunch of emails: thanks for bearing with me 😅

lionel-nj · 2021-04-14T15:26:59Z

As 5b96df7 demonstrates. the inclusion of "[ci skip]" key word in a commit message will prevent the execution of the integration test workflow.

lionel-nj · 2021-04-14T16:03:47Z

Build the CLI project with the latest changes and run the gtfs-validator JAR in the GitHub action (i.e., what we're currently doing)

This requires being able to run the validator without providing -f CLI arg. #851 has been created to this extent.

Download the latest release JAR from https://github.com/MobilityData/gtfs-validator/releases (GitHub might be able to do this directly vs. making an HTTP request?) and run it

For now this is done via Github action

We'll need to make sure the two sets of output don't collide.

#852 makes reports names user configurable so that we can make sure reports do not collide

lionel-nj · 2021-04-27T14:55:32Z

Per trial and error, the value passed to DATASETS should be a compact stringified json object such as:

{"include":[{"url":"http://webapps.thebus.org/transitdata/Production/google_transit.zip","output":"thb"},{"url":"http://www.transperth.wa.gov.au/TimetablePDFs/GoogleTransit/Production/google_transit.zip","output":"transperth"},{"url":"https://octa.net/current/google_transit.zip","output":"octa"}]}

main/build.gradle

comparator/build.gradle

lionel-nj · 2021-04-29T20:49:41Z

Thanks for the advice @barbeau! That worked: 562ec00 💯

aababilov

Thanks, some first comments regarding the code style. I have not checked the overall logic yet.

comparator/src/main/java/org/mobilitydata/gtfsvalidator/outputcomparator/cli/Main.java

...ator/src/main/java/org/mobilitydata/gtfsvalidator/outputcomparator/util/NoticeAggregate.java

...tor/src/main/java/org/mobilitydata/gtfsvalidator/outputcomparator/util/ValidationReport.java

lionel-nj · 2021-05-11T14:18:14Z

Thanks @aababilov - c4e3459 introduces changes to reduce the complexity of getNewErrorCount.

I encapsulated ValidationReport in ValidationReportContainer so that the set of error codes is generated only once: we no longer have to create this set each time getErrorCodes is called.

You still have code duplication and you do not close your latestReportReader.

ValidationReportContainer now implements AutoCloseable so that is each container is automatically closed after usage in the try-catch block.
ValidationReportContainer has method fromPath to avoid code duplication in the main method.

But why double?

My bad, it is now an int as it should have been from the beginning.

aababilov · 2021-05-11T20:01:51Z

Thanks for updates, Lionel! And thanks for fixing the performance.

lionel-nj · 2021-05-12T14:52:46Z

I am not sure that I see how this class helps to simplify the code. Instead, it mixes some unrelated logic:

reading of ValidationReport - with holding the input Reader object open;
caching reportErrorCodes that logically belong to ValidationReport

Indeed, that was confusing - I used GSON custom deserialization instead of using the proxy ValidationReportContainer. Now all the logic is in ValidationReport whose construction process has been clarified.

Why hold the reader open after we read all the report?

The latest update of this PR closes the reader right after reading the files.

This description of "return" is too long and it repeats the information above.

Modified.

@aababilov PTAL

.github/workflows/integration_test.yml

barbeau

@lionel-nj Thanks for working on this! Some feedback in-line below.

.github/workflows/integration_test.yml

README.md

docs/INTEGRATION_TESTS.md

barbeau · 2021-05-27T21:30:57Z

...tor/src/main/java/org/mobilitydata/gtfsvalidator/outputcomparator/util/ValidationReport.java

+   *     as parameter.
+   */
+  public int getNewErrorCount(ValidationReport other) {
+    return Sets.difference(other.getErrorCodes(), getErrorCodes()).size();


I think we want to know about a change in the other direction too. For example, if the last validator release found 4 error types, and the latest snapshot only found 2 error types (and presumably a rule implementation changed), that's going to allow new data that was previously invalid. My understanding is that the current implementation doesn't catch this due to order of variables here.

Should we compare both ways, and return a positive or negative value depending on the direction of change?

As discussed during our last meeting: this could be nice but not a priority now. @isabelle-dr will come back to us with more details about the possible use cases needed for these acceptance tests.

In order to keep track of this, I will leave this discussion open for now. Will revisit in the future if needed.

...tor/src/main/java/org/mobilitydata/gtfsvalidator/outputcomparator/util/ValidationReport.java

.../java/org/mobilitydata/gtfsvalidator/outputcomparator/util/ValidationReportDeserializer.java

comparator/src/main/java/org/mobilitydata/gtfsvalidator/outputcomparator/cli/Main.java

lionel-nj · 2021-05-31T20:51:44Z

@barbeau thanks for reviewing!

One question: how would you recommend proceeding to execute this new workflow only when file from package org.mobilitydata.gtfsvalidator.validator are changed?

I tried to leverage paths:

on:
  push:
    branches: [ master, new-test-module ]
    paths:
      - 'main/src/main/java/org/mobilitydata/gtfsvalidator/validator'
      - 'core/src/main/java/org/mobilitydata/gtfsvalidator/validator'

on:
  push:
    branches: [ master, new-test-module ]
    paths:
      - '../../main/src/main/java/org/mobilitydata/gtfsvalidator/validator'
      - '../../core/src/main/java/org/mobilitydata/gtfsvalidator/validator'

but these attempts were not successful.

barbeau · 2021-06-01T14:15:19Z

@lionel-nj I think you're just missing the wildcard?

Try something like:

on:
  push:
    branches: [ master, new-test-module ]
    paths:
      - 'main/src/main/java/org/mobilitydata/gtfsvalidator/validator/**'
      - 'core/src/main/java/org/mobilitydata/gtfsvalidator/validator/**'

For example see https://help.sumologic.com/03Send-Data/Sources/04Reference-Information-for-Sources/Using-Wildcards-in-Paths.

lionel-nj · 2021-06-01T14:26:10Z

That worked! Thank you @barbeau

barbeau

Looking good @lionel-nj! Some comments in-line.

.github/workflows/acceptance_test.yml

output-comparator/README.md

docs/ACCEPTANCE_TESTS.md

output-comparator/src/main/java/org/mobilitydata/gtfsvalidator/outputcomparator/cli/Main.java

lionel-nj · 2021-06-11T14:07:28Z

Thanks for reviewing. After discussion with @isabelle-dr: this CI process should be executed on any code change. Hence, I removed the support to only execute this workflow when changes were provided to validator module.

barbeau · 2021-06-11T14:57:06Z

After discussion with @isabelle-dr: this CI process should be executed on any code change.

Ok - you could still set it to ignore changes to .md files like the main CI

core/src/main/java/org/mobilitydata/gtfsvalidator/model/ValidationReport.java

docs/ACCEPTANCE_TESTS.md

...-comparator/src/main/java/org/mobilitydata/gtfsvalidator/outputcomparator/cli/Arguments.java

output-comparator/src/main/java/org/mobilitydata/gtfsvalidator/outputcomparator/cli/Main.java

...t-comparator/src/test/java/org/mobilitydata/gtfsvalidator/outputcomparator/cli/MainTest.java

asvechnikov2 · 2021-12-22T02:36:36Z

Ugh... I tried to add quote reply and accidently sent the whole review...

How would such test differ from the unit test provided in MainTest? From my understanding MainTest tests the comparison process. Do you think that these integration tests should test the entire Github pipeline?

We have pretty good coverage of unit tests, however, if we look at the overall feature it does the next

Fetches list of urls
Fetches individual datasets
Runs validation on the datasets and stores results
Runs reports comparison
Updates GitHub according to comparison

We're testing almost each step, but we don't know if each step is correctly linked to the next one. Once we make an update to the code this linking could be broken and we should have a way to make sure everything works fine. This could be just an instruction on how to run the whole pipeline and assess its results, unfortunately, I didn't have time to look at the way how GitHub pipelines could be tested.

lionel-nj · 2021-12-23T14:07:36Z

We're testing almost each step, but we don't know if each step is correctly linked to the next one. Once we make an update to the code this linking could be broken and we should have a way to make sure everything works fine. This could be just an instruction on how to run the whole pipeline and assess its results, unfortunately, I didn't have time to look at the way how GitHub pipelines could be tested.

One thing that could be done to test the pipeline's execution would be allowing download of the validation reports from the Google Storage Bucket that we use. We also have an internal notebook that is used to compute information about the state of datasets - which could be leveraged for the sakes of verification. We could integrate and document this verification process in a subsequent step. What do you think about that @asvechnikov2?

Edit: actually all validation reports are already available in the artifacts persisted after execution of the pipeline (therefore, no need to open the Google Storage bucket to the public). So I will update the documentation with basic instructions to verify the execution of the acceptance test. We could still provide a notebook to automate the task in the future. @asvechnikov2 @isabelle-dr

cc @isabelle-dr

- generate source corruption report - refactor MainTest.writeFile method - refactor ValidationReport - implement resolve for code clarity and consistency - additional unit tests - clarify documentation

github-actions · 2021-12-23T16:10:38Z

Thank you for this contribution.

Information about source corruption

0 out of 1247 sources are corrupted.
The following sources are corrupted:

Acceptance test details

Also, the changes in this pull request did not trigger any new errors on known GTFS datasets from the MobilityDatabase.
Download the full acceptance test report for commit 6a01baa here (report will disappear after 90 days).

github-actions · 2021-12-23T16:39:34Z

Thank you for this contribution! 🍰✨🦄

Information about source corruption

0 out of 1247 sources are corrupted.

Acceptance test details

The changes in this pull request did not trigger any new errors on known GTFS datasets from the MobilityDatabase.
Download the full acceptance test report for commit 6eb4f45 here (report will disappear after 90 days).

isabelle-dr · 2021-12-23T22:03:59Z

The emoji choice is on point 👌

asvechnikov2

Thanks! LGTM!

actually all validation reports are already available in the artifacts persisted after execution of the pipeline (therefore, no need to open the Google Storage bucket to the public)

I saw messages from github-actions that reflect new changes, so it seems that it's possible to start the pipeline with new changes and verify its results manually. I think this should be enough to verify that everything works the way it's expected, so there's no need for any automation. We might want to add one broken feed to the sources to make sure that the pipeline catches and correctly processes this use case.

core/src/main/java/org/mobilitydata/gtfsvalidator/model/ValidationReport.java

asvechnikov2 · 2021-12-29T07:05:24Z

scripts/comment_generator.py

+    comment = (
+        "Thank you for this contribution! 🍰✨🦄 \n\n"
+        "### Information about source "
+        "corruption \n\n"
+        f"{corrupted_sources_report['corruptedSourcesCount']} out of "
+        f"{corrupted_sources_report['sourceIdCount']}"
+        f" sources are corrupted."
+    )


This information snippet looks really great! No need to make any additional changes right now, I just want a feature request, to provide information about how many source there were, how many broken, how many newly broken, how many corrupted, etc.

I opened #1085 to follow up on the next steps. Feel free to comment there if you see something missing. Thank you again for all the precious feedback 🙏

- remove unused variable

github-actions · 2022-01-10T12:01:25Z

Thank you for this contribution! 🍰✨🦄

Information about source corruption

0 out of 1248 sources are corrupted.

Acceptance test details

The changes in this pull request did not trigger any new errors on known GTFS datasets from the MobilityDatabase.
Download the full acceptance test report for commit 2d320f2 here (report will disappear after 90 days).

lionel-nj · 2022-01-10T12:08:12Z

After providing the last modifications, I am super excited about merging this PR. Thank you very much to everyone involved in the reflexion and the PR review process (@aababilov, @asvechnikov2, @barbeau, @isabelle-dr, @maximearmstrong).

isabelle-dr · 2022-01-10T12:49:01Z

Amazing work @lionel-nj 👏👏👏
Massive Kudos for bringing this feature to the (first) finish line :)

barbeau · 2022-01-10T16:18:07Z

Congrats @lionel-nj for all your work on this!

f8full · 2022-01-11T03:24:01Z

Bravo @lionel-nj

maximearmstrong · 2022-01-11T14:15:59Z

Great work @lionel-nj ! Congrats 🙌 🚀

lionel-nj self-assigned this Apr 9, 2021

barbeau reviewed Apr 9, 2021

View reviewed changes

integration/src/main/java/org/mobilitydata/gtfsvalidator/integration/Main.java Outdated Show resolved Hide resolved

barbeau reviewed Apr 29, 2021

View reviewed changes

main/build.gradle Outdated Show resolved Hide resolved

barbeau reviewed Apr 29, 2021

View reviewed changes

comparator/build.gradle Outdated Show resolved Hide resolved

barbeau reviewed Apr 29, 2021

View reviewed changes

comparator/build.gradle Outdated Show resolved Hide resolved

lionel-nj marked this pull request as ready for review April 29, 2021 21:03

lionel-nj requested a review from barbeau April 29, 2021 21:03

lionel-nj mentioned this pull request May 5, 2021

feat: frequencies.txt exact_times=0 trips must not have stop_times.timepoint=1 records #887

Closed

3 tasks

lionel-nj requested a review from aababilov May 5, 2021 15:45

aababilov requested changes May 5, 2021

View reviewed changes

lionel-nj requested a review from aababilov May 11, 2021 14:18

barbeau reviewed May 21, 2021

View reviewed changes

.github/workflows/integration_test.yml Outdated Show resolved Hide resolved

barbeau reviewed May 27, 2021

View reviewed changes

lionel-nj changed the title ~~feat: create additional module for integration tests~~ feat: create additional module for acceptance tests May 31, 2021

lionel-nj requested a review from barbeau May 31, 2021 20:51

barbeau reviewed Jun 10, 2021

View reviewed changes

lionel-nj requested a review from barbeau June 11, 2021 17:56

document that ValidationReport only stores errors

f540035

lionel-nj requested a review from asvechnikov2 December 21, 2021 17:28

asvechnikov2 reviewed Dec 22, 2021

View reviewed changes

lionel-nj added 3 commits December 23, 2021 11:17

apply suggestion from code review:

190720d

- generate source corruption report - refactor MainTest.writeFile method - refactor ValidationReport - implement resolve for code clarity and consistency - additional unit tests - clarify documentation

update workflow with path to source_corruption_report.json

28935a6

fix path

3614ba3

only list corrupted sources when present add some glitters ✨

092571b

update doc [acceptance test skip]

7d1e454

lionel-nj requested a review from asvechnikov2 December 23, 2021 20:11

asvechnikov2 approved these changes Dec 29, 2021

View reviewed changes

isabelle-dr mentioned this pull request Jan 7, 2022

Additional information and better logic for the acceptance test feature #1085

Open

apply suggestions from code review

2dadbae

- remove unused variable

apply goJF [acceptance test skip]

5ef30ad

MobilityData deleted a comment from github-actions bot Jan 10, 2022

lionel-nj merged commit fed4bad into master Jan 10, 2022

lionel-nj deleted the new-test-module branch January 10, 2022 12:24

barbeau mentioned this pull request Jan 11, 2022

Improve text describing "corrupted sources" in acceptance test results #1087

Open

barbeau mentioned this pull request Feb 1, 2022

Acceptance tests fail for PR opened from forked repository #1094

Closed

isabelle-dr mentioned this pull request Aug 10, 2022

Improve acceptance rules #1232

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: acceptance tests #848

feat: acceptance tests #848

lionel-nj commented Apr 9, 2021 •

edited

Loading

lionel-nj commented Apr 14, 2021 •

edited

Loading

lionel-nj commented Apr 14, 2021

lionel-nj commented Apr 14, 2021

lionel-nj commented Apr 27, 2021 •

edited

Loading

lionel-nj commented Apr 29, 2021

aababilov left a comment

lionel-nj commented May 11, 2021 •

edited

Loading

aababilov commented May 11, 2021

lionel-nj commented May 12, 2021 •

edited

Loading

barbeau left a comment

barbeau May 27, 2021

lionel-nj May 31, 2021

lionel-nj commented May 31, 2021

barbeau commented Jun 1, 2021

lionel-nj commented Jun 1, 2021

barbeau left a comment

lionel-nj commented Jun 11, 2021

barbeau commented Jun 11, 2021

asvechnikov2 commented Dec 22, 2021

lionel-nj commented Dec 23, 2021 •

edited

Loading

github-actions bot commented Dec 23, 2021

github-actions bot commented Dec 23, 2021

isabelle-dr commented Dec 23, 2021

asvechnikov2 left a comment

asvechnikov2 Dec 29, 2021

isabelle-dr Jan 7, 2022

github-actions bot commented Jan 10, 2022

lionel-nj commented Jan 10, 2022 •

edited

Loading

isabelle-dr commented Jan 10, 2022 •

edited

Loading

barbeau commented Jan 10, 2022

f8full commented Jan 11, 2022

maximearmstrong commented Jan 11, 2022

feat: acceptance tests #848

feat: acceptance tests #848

Conversation

lionel-nj commented Apr 9, 2021 • edited Loading

Input data:

Comparison process

Final output

Error handling

lionel-nj commented Apr 14, 2021 • edited Loading

lionel-nj commented Apr 14, 2021

lionel-nj commented Apr 14, 2021

lionel-nj commented Apr 27, 2021 • edited Loading

lionel-nj commented Apr 29, 2021

aababilov left a comment

Choose a reason for hiding this comment

lionel-nj commented May 11, 2021 • edited Loading

aababilov commented May 11, 2021

lionel-nj commented May 12, 2021 • edited Loading

barbeau left a comment

Choose a reason for hiding this comment

barbeau May 27, 2021

Choose a reason for hiding this comment

lionel-nj May 31, 2021

Choose a reason for hiding this comment

lionel-nj commented May 31, 2021

barbeau commented Jun 1, 2021

lionel-nj commented Jun 1, 2021

barbeau left a comment

Choose a reason for hiding this comment

lionel-nj commented Jun 11, 2021

barbeau commented Jun 11, 2021

asvechnikov2 commented Dec 22, 2021

lionel-nj commented Dec 23, 2021 • edited Loading

github-actions bot commented Dec 23, 2021

Information about source corruption

Acceptance test details

github-actions bot commented Dec 23, 2021

Information about source corruption

Acceptance test details

isabelle-dr commented Dec 23, 2021

asvechnikov2 left a comment

Choose a reason for hiding this comment

asvechnikov2 Dec 29, 2021

Choose a reason for hiding this comment

isabelle-dr Jan 7, 2022

Choose a reason for hiding this comment

github-actions bot commented Jan 10, 2022

Information about source corruption

Acceptance test details

lionel-nj commented Jan 10, 2022 • edited Loading

isabelle-dr commented Jan 10, 2022 • edited Loading

barbeau commented Jan 10, 2022

f8full commented Jan 11, 2022

maximearmstrong commented Jan 11, 2022

lionel-nj commented Apr 9, 2021 •

edited

Loading

lionel-nj commented Apr 14, 2021 •

edited

Loading

lionel-nj commented Apr 27, 2021 •

edited

Loading

lionel-nj commented May 11, 2021 •

edited

Loading

lionel-nj commented May 12, 2021 •

edited

Loading

lionel-nj commented Dec 23, 2021 •

edited

Loading

lionel-nj commented Jan 10, 2022 •

edited

Loading

isabelle-dr commented Jan 10, 2022 •

edited

Loading