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

UnitTests fail on windows due to path formatting problems #2855

Merged
merged 16 commits into from
Aug 7, 2023

Conversation

ludfjig
Copy link
Contributor

@ludfjig ludfjig commented Aug 1, 2023

What does this change

Lots of tests fail on windows because paths are hardcoded using forward slash, and never expects a volume such as "C:".

What issue does it fix

Closes #2854 but leaves #2858 for later

Checklist

  • Did you write tests?
  • Did you write documentation?
  • Did you change porter.yaml or a storage document record? Update the corresponding schema file.
  • If this is your first pull request, please add your name to the bottom of our Contributors list. Thank you for making Porter better! 🙇‍♀️

Reviewer Checklist

  • Comment with /azp run test-porter-release if a magefile or build script was modified
  • Comment with /azp run porter-integration if it's a non-trivial PR

@ludfjig ludfjig changed the title Tests fail on windows due to path formatting problems UnitTests fail on windows due to path formatting problems Aug 4, 2023
@ludfjig ludfjig marked this pull request as ready for review August 4, 2023 23:35
pkg/exec/builder/execute_test.go Outdated Show resolved Hide resolved
{"uninstall", "uninstall", "/home/myuser/.porter/mixins/exec/exec uninstall"},
{"invoke", "status", "/home/myuser/.porter/mixins/exec/exec invoke --action status"},
{"version", "version --output json", "/home/myuser/.porter/mixins/exec/exec version --output json"},
{"build", "build", "/home/myuser/.porter/mixins/exec/exec build\n\\home\\myuser\\.porter\\mixins\\exec\\exec.exe build"},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is happening here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can separate expected output by a newline character, so for this test there are two possible expected outputs which both pass

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh interesting, might be easier to read if it is separated into array for the want command? maybe it isn't possible, not sure how the testing framework works.

pkg/porter/cnab_test.go Outdated Show resolved Hide resolved
pkg/porter/credentials_test.go Outdated Show resolved Hide resolved
pkg/porter/credentials_test.go Outdated Show resolved Hide resolved
@@ -225,6 +224,11 @@ func TestPublish_RefreshCachedBundle(t *testing.T) {
require.NoError(t, err)
origBunPathTime := file.ModTime()

if runtime.GOOS == "windows" {
// see https://github.com/getporter/porter/issues/2858
time.Sleep(5 * time.Millisecond)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:-( we've seen this in k8s too

pkg/test/helper.go Outdated Show resolved Hide resolved
@jsturtevant
Copy link

Will you add a github job that runs the unit tests on prs? (Maybe as a follow up?)

ludfjig added 15 commits August 7, 2023 11:33
Signed-off-by: Ludvig Liljenberg <[email protected]>
Signed-off-by: Ludvig Liljenberg <[email protected]>
Signed-off-by: Ludvig Liljenberg <[email protected]>
Signed-off-by: Ludvig Liljenberg <[email protected]>
Signed-off-by: Ludvig Liljenberg <[email protected]>
Signed-off-by: Ludvig Liljenberg <[email protected]>
Signed-off-by: Ludvig Liljenberg <[email protected]>
Signed-off-by: Ludvig Liljenberg <[email protected]>
Signed-off-by: Ludvig Liljenberg <[email protected]>
Signed-off-by: Ludvig Liljenberg <[email protected]>
Signed-off-by: Ludvig Liljenberg <[email protected]>
Signed-off-by: Ludvig Liljenberg <[email protected]>
Signed-off-by: Ludvig Liljenberg <[email protected]>
Signed-off-by: Ludvig Liljenberg <[email protected]>
Signed-off-by: Ludvig Liljenberg <[email protected]>
Signed-off-by: Ludvig Liljenberg <[email protected]>
@schristoff
Copy link
Member

/azp run porter-integration

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@schristoff schristoff enabled auto-merge (squash) August 7, 2023 20:03
@schristoff schristoff disabled auto-merge August 7, 2023 20:03
@schristoff schristoff merged commit 34ad0d1 into getporter:main Aug 7, 2023
@ludfjig ludfjig deleted the windows_paths branch August 7, 2023 20:35
AGMETEOR pushed a commit to AGMETEOR/porter that referenced this pull request Aug 9, 2023
…2855)

* (feat) Fix Unit Tests to pass on Windows

Signed-off-by: Ludvig Liljenberg <[email protected]>

---------

Signed-off-by: Ludvig Liljenberg <[email protected]>
Signed-off-by: Allan Guwatudde <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: test fails when running on windows due to path formatting
3 participants