test: Add path parsing tests #1155

barbeau · 2022-05-18T15:11:59Z

Summary:

In PR #1146 we observed that changes were made to the input GTFS file path handling that broke execution (CLI and the GUI) on Windows. However, CI never caught this because currently all end-to-end tests are run only on ubuntu-latest.

~~This PR changes the CI configuration to run the smallest end-to-end test on windows-latest as well.~~

Given the complexity that's emerged of running end-to-end Windows CI (see below comments), I've changed this PR to add simple unit tests to catch the same issue (#1158).

This should catch future instances of path issues on Windows prior to PR merges.

Expected behavior:

~~CI should catch changes that break end-to-end functionality on Windows as well as Linux.~~

Unit test should catch path parsing errors

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)

Currently, project version info is passed in via a `versionTag` env variable, which is only specified in the Github workflow environment. Consequently, `project.version` ends up being `null` when run locally. This causes weirdness for tools that expect a valid version number. This change switches the project to use the axion-release plugin, which determines the project version by looking at the git repo's tags. Along with that switch, I've moved the project group and version specification into the root build.gradle allprojects {} such that these values do not need to be specified in each sub-poject. I also did some related clean-up for our jitpack configuration (and Maven repository modules, should we every decide to publish them). I believe our current jitpack config doesn't actually work correctly because we only publish artifacts from the main package, but not core. You can see this in practice when looking at: https://jitpack.io/com/github/MobilityData/gtfs-validator/3.0.1/gtfs-validator-3.0.1.pom Notice how it has a dependency on the `core` module but we don't actually publish it anywhere. If you actually attempt to use our project as a jitpack dependency, it will fail. The change was to also publish the core module and to specify more natural artifact names.

longer needed now that project version is determined within Gradle directly. Also fixup references to shadow jars, since they no longer include the versionTag in the filename (also _cli => -cli).

We can't update this code until the PR actually goes in on the master branch.

…flicts.

barbeau · 2022-05-18T15:18:48Z

The Windows end-to-end workflow currently fails because of the version tag script:
https://github.com/MobilityData/gtfs-validator/runs/6492006893

VERSION_TAG=edge
  if [[ $GITHUB_REF == refs/tags/* ]]; then
    VERSION_TAG=${GITHUB_REF#refs/tags/}
  elif [[ $GITHUB_REF == refs/heads/* ]]; then
    VERSION_TAG=-$(echo ${GITHUB_REF#refs/heads/} | sed -r 's#/+#-#g')
  elif [[ $GITHUB_REF == refs/pull/* ]]; then
    VERSION_TAG=-pr-1155
  fi
  if [ ${VERSION_TAG} != ${GITHUB_REF#refs/tags/} ]; then
    VERSION_TAG=v${VERSION_TAG}-sha-${GITHUB_SHA::8}-SNAPSHOT
  fi
  echo ::set-output name=versionTag::${VERSION_TAG}
  shell: C:\Program Files\PowerShell\7\pwsh.EXE -command ". '{0}'"
ParserError: D:\a\_temp\92c1c[3](https://github.com/MobilityData/gtfs-validator/runs/6492006893?check_suite_focus=true#step:3:3)78-c3da-4a31-9920-9071a4c0ff72.ps1:3
Line |
   3 |  if [[ $GITHUB_REF == refs/tags/* ]]; then
     |    ~
     | Missing '(' after 'if' in if statement.

Error: Process completed with exit code 1.

This script is removed in PR #1147 as part of the refactor of version number handling, so let's revisit this PR after PR #1147 is merged, as that PR should resolve the issue.

github-actions · 2022-05-18T15:51:44Z

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 3e3b508 here (report will disappear after 90 days).

I also cleaned up some other variables from the docker config that don't seem to be used.

…previous commit.

# Conflicts: # .github/workflows/end_to_end.yml

github-actions · 2022-05-23T21:38:51Z

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 e99fa25 here (report will disappear after 90 days).

…inux" This reverts commit 31f95a0.

barbeau · 2022-05-24T19:34:56Z

So apparently running the validator JAR file with wildcards like:

java -jar main/build/libs/gtfs-validator-*.jar ...

...doesn't work on Windows PowerShell.

Here's the failed CI run with PowerShell:
https://github.com/MobilityData/gtfs-validator/runs/6562700554

...with the error:

Run java -jar main/build/libs/gtfs-validator-*.jar --url https://openmobilitydata.org/p/transport-for-nsw/237/latest/download --output_base output --country_code au --storage_directory gs.zip
  java -jar main/build/libs/gtfs-validator-*.jar --url https://openmobilitydata.org/p/transport-for-nsw/[2](https://github.com/MobilityData/gtfs-validator/runs/6562700554?check_suite_focus=true#step:6:2)[3](https://github.com/MobilityData/gtfs-validator/runs/6562700554?check_suite_focus=true#step:6:3)7/latest/download --output_base output --country_code au --storage_directory gs.zip
  shell: C:\Program Files\PowerShell\7\pwsh.EXE -command ". '{0}'"
  env:
    JAVA_HOME: C:\hostedtoolcache\windows\Java_Temurin-Hotspot_jdk\11.0.15-10\x6[4](https://github.com/MobilityData/gtfs-validator/runs/6562700554?check_suite_focus=true#step:6:4)
Error: Unable to access jarfile main/build/libs/gtfs-validator-*.jar
Error: Process completed with exit code 1.

I can reproduce this locally on Windows 10 Home w/ PowerShell too.

I tried switching to using shell: bash on Windows, and this fixes the wildcard issue.

However, using bash hides the path issue that we saw in #1158 (i.e., the path issue only happens when using PowerShell) and defeats the purpose of having a separately Windows CI task. Here's a successful CI run when it should have failed due to the path handling bug (which I added to this PR to test):
https://github.com/MobilityData/gtfs-validator/runs/6579581437

@bdferris-v2 Based on your work in PR #1147 do you have ideas for the most elegant way to handle this lack of wildcard expansion on Windows PowerShell?

Some options I can think of:

Change the GitHub Action script to run different commands for Linux and Windows to handle wildcard expansion on Windows - Pro is no changes to the app, con is that it's a bit messy for CI.
Pass the version number from Gradle build to CI (e.g., via environmental variable) so the exact JAR file name can be executed on CI - Pro is CI script remains same across platforms, con is changes required to the app.

bdferris-v2 · 2022-05-25T04:10:28Z

First, a meta question:

Is there really value in running the entire suite of end-to-end feed tests on both Windows and Linux? I think the general classes of errors we are catching here ("does this feed cause the validator to crash unexpectedly?") will be the same on both platforms. Instead, there's an alternate approach where we have a single specific Windows test with a single feed that's just designed to capture platform quirks of Windows.

Unfortunately, even if you buy that argument, I'm not sure how much it actually buys us because we'd still need to duplicate major portions of the job definition for Windows.

So on to some alternatives. A few that I can think of:

We use the axion-release plugin option to force override the version aka -Prelease.forceVersion=1.0.0 such that the produced jar has a consistent version number that can be hard-coded in the test. I think this would mostly work, except the plugin still appends -SNAPSHOT for non-release runs. Thus, I think this would break under an actual release build (where axion would helpfully drop the -SNAPSHOT), thus we are back needing a wildcard...
We try to extract the version using the ./gradlew currentVersion command. More specifically, we'd use the concise output version: ./gradlew cV -q -Prelease.quiet. That could be a pre-run step and then used to specify the command-line. I think this should work?
We use a run step to construct the command-line for the jar with some os-specific logic as a pre-step before running all the actual validation runs.
We switch the shell to bash for all environments. I know it's not ideal, but I do believe it would have still caught When using CLI in windows, invalid path exception is always thrown #1158 even in that configuration.

github-actions · 2022-05-25T15:54:49Z

This contribution does not follow the conventions set by the Google Java style guide. Please run the following command line at the root of the project to fix formatting errors: ./gradlew goJF.

github-actions · 2022-05-25T15:56:27Z

This contribution does not follow the conventions set by the Google Java style guide. Please run the following command line at the root of the project to fix formatting errors: ./gradlew goJF.

github-actions · 2022-05-25T15:57:12Z

This contribution does not follow the conventions set by the Google Java style guide. Please run the following command line at the root of the project to fix formatting errors: ./gradlew goJF.

barbeau · 2022-05-25T16:07:07Z

Is there really value in running the entire suite of end-to-end feed tests on both Windows and Linux?

Thanks @bdferris-v2, that's a good point. That got me thinking - given these issues, I think a simple unit test for path parsing is probably the best/simplest approach here that gives us coverage for #1158.

I added such a test in e64e234 and removed the end-to-end Windows CI from this PR (we still run tests on Windows CI, though). I intentionally reintroduced the bug from #1158 here in this PR to see how the CI handles it. Hopefully it fails as it should. If that works, I'll remove the bug and hopefully we'll have green lights.

Thoughts and feedback are welcome.

barbeau · 2022-05-25T16:21:06Z

Ok, looks good. With the bug from #1158 still in this PR, the new unit test is failing on Windows as expected:
https://github.com/MobilityData/gtfs-validator/runs/6595978098

...but passing on Linux:
https://github.com/MobilityData/gtfs-validator/runs/6595977965

I just removed the #1158 bug from this PR in 62d434d, so hopefully everything should be green on CI now (I tested it locally on Windows and it works).

This PR should be ready for review.

barbeau · 2022-05-25T17:23:10Z

Looks like the acceptance tests are failing:
https://github.com/MobilityData/gtfs-validator/runs/6596556690

...with:

Run actions/download-artifact@v2
Starting download for gtfs-validator-master
Error: Unable to find an artifact with the name: gtfs-validator-master

@bdferris-v2 Is this related to the below TODO statement in acceptance_test.yml?

    steps:
      - uses: actions/checkout@v1
        with:
          ref: master
      # We still need to compute the versionTag for the master branch until
      # changes in PR #1147 have gone in, at which point we can clean up this
      # codepath to match the snapshot.
      - name: Prepare version name

bdferris-v2 · 2022-05-25T17:25:21Z

Ah, yes! I had meant to fast-follow on that because of exactly this issue, but got distracted. Let me get a PR in to fix that.

barbeau · 2022-05-25T17:26:06Z

Awesome, thanks!

bdferris-v2 · 2022-05-25T18:41:52Z

@barbeau tracking the fix in PR #1168.

bdferris-v2

LGTM

github-actions · 2022-05-25T23:36:20Z

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 34b4b08 here (report will disappear after 90 days).

maximearmstrong

LGTM too! Thank you @barbeau

bdferris-v2 and others added 6 commits May 13, 2022 23:42

Remove references to versionTag env variable in workflows. It is no

9e17ab3

longer needed now that project version is determined within Gradle directly. Also fixup references to shadow jars, since they no longer include the versionTag in the filename (also _cli => -cli).

Restore versionTag behavior for master branch.

c53da0d

We can't update this code until the PR actually goes in on the master branch.

Fix typo in yml.

f82ca27

Download artifacts to separate directories to avoid eventual name con…

44a4bd8

…flicts.

ci: Add end-to-end Windows check

be32036

barbeau requested review from maximearmstrong and isabelle-dr May 18, 2022 15:12

barbeau assigned maximearmstrong and isabelle-dr May 18, 2022

barbeau mentioned this pull request May 18, 2022

feat: Initial support for validation app GUI #1146

Merged

barbeau marked this pull request as draft May 18, 2022 15:14

barbeau mentioned this pull request May 19, 2022

fix: Correct GTFS input path handling on Windows #1160

Closed

4 tasks

bdferris-v2 and others added 6 commits May 21, 2022 20:32

Remove all references to versionTag from docker.yml

8ade38c

I also cleaned up some other variables from the docker config that don't seem to be used.

Remove all references to versionTag from test_pack_doc.yml

d65691d

Add back tags output variable, which was improperly removed in the …

2d4c313

…previous commit.

Merge branch 'issue/1139/versions' into sean/ci-windows-end-to-end

fe6593c

# Conflicts: # .github/workflows/end_to_end.yml

Merge branch 'master' into sean/ci-windows-end-to-end

5bcc6b7

fix: Use bash on Windows (instead of PowerShell) as well as Linux

31f95a0

barbeau added 3 commits May 24, 2022 14:37

break: Intentionally break path handling on Windows to test CI

f928395

Merge branch 'master' into sean/ci-windows-end-to-end

1d2175d

Revert "fix: Use bash on Windows (instead of PowerShell) as well as L…

0900ce2

…inux" This reverts commit 31f95a0.

maximearmstrong added this to This Sprint in The Tech Dashboard (archived) May 25, 2022

test: Add path parsing tests

e64e234

ci: Remove Windows end-to-end CI job

51a108d

chore: Fix whitespace

65afd7d

barbeau changed the title ~~ci: Add end-to-end Windows check~~ test: Add path parsing tests May 25, 2022

chore: Fix code formatting with ./gradlew goJF

629e64d

barbeau added 2 commits May 25, 2022 12:08

docs: Add issue reference

45884db

fix: Remove the bug from #1158 re-introduced in this PR for testing

62d434d

barbeau marked this pull request as ready for review May 25, 2022 16:21

barbeau requested a review from bdferris-v2 May 25, 2022 16:21

bdferris-v2 approved these changes May 25, 2022

View reviewed changes

The Tech Dashboard (archived) automation moved this from This Sprint to Approved May 25, 2022

Merge branch 'master' into sean/ci-windows-end-to-end

98d6ab8

maximearmstrong approved these changes May 26, 2022

View reviewed changes

maximearmstrong merged commit 42c6dd3 into master May 26, 2022

The Tech Dashboard (archived) automation moved this from Approved to Done May 26, 2022

maximearmstrong deleted the sean/ci-windows-end-to-end branch May 26, 2022 15:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Add path parsing tests #1155

test: Add path parsing tests #1155

barbeau commented May 18, 2022 •

edited

barbeau commented May 18, 2022 •

edited

github-actions bot commented May 18, 2022

github-actions bot commented May 23, 2022

barbeau commented May 24, 2022 •

edited

bdferris-v2 commented May 25, 2022

github-actions bot commented May 25, 2022

github-actions bot commented May 25, 2022

github-actions bot commented May 25, 2022

barbeau commented May 25, 2022

barbeau commented May 25, 2022 •

edited

barbeau commented May 25, 2022

bdferris-v2 commented May 25, 2022

barbeau commented May 25, 2022

bdferris-v2 commented May 25, 2022

bdferris-v2 left a comment

github-actions bot commented May 25, 2022

maximearmstrong left a comment

test: Add path parsing tests #1155

test: Add path parsing tests #1155

Conversation

barbeau commented May 18, 2022 • edited

barbeau commented May 18, 2022 • edited

github-actions bot commented May 18, 2022

Information about source corruption

Acceptance test details

github-actions bot commented May 23, 2022

Information about source corruption

Acceptance test details

barbeau commented May 24, 2022 • edited

bdferris-v2 commented May 25, 2022

github-actions bot commented May 25, 2022

github-actions bot commented May 25, 2022

github-actions bot commented May 25, 2022

barbeau commented May 25, 2022

barbeau commented May 25, 2022 • edited

barbeau commented May 25, 2022

bdferris-v2 commented May 25, 2022

barbeau commented May 25, 2022

bdferris-v2 commented May 25, 2022

bdferris-v2 left a comment

Choose a reason for hiding this comment

github-actions bot commented May 25, 2022

Information about source corruption

Acceptance test details

maximearmstrong left a comment

Choose a reason for hiding this comment

barbeau commented May 18, 2022 •

edited

barbeau commented May 18, 2022 •

edited

barbeau commented May 24, 2022 •

edited

barbeau commented May 25, 2022 •

edited