Skip to content

RSDK-13980: Enable cli windows tests#6022

Open
allisonschiang wants to merge 18 commits into
mainfrom
cli-windows-testing
Open

RSDK-13980: Enable cli windows tests#6022
allisonschiang wants to merge 18 commits into
mainfrom
cli-windows-testing

Conversation

@allisonschiang
Copy link
Copy Markdown
Member

@allisonschiang allisonschiang commented May 18, 2026

This PR enables cli in the windows testing suite and then fixes some of the tests to make it work for windows. This PR only fixes tests, not broken implementations, and I added skips to tests with the jira ticket documenting that the original command doesn't work.

Testing using the github workflow that enables it:
https://github.com/viamrobotics/rdk/actions/runs/26180155038

@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels May 18, 2026
allisonschiang and others added 13 commits May 19, 2026 16:10
…onfig+TestLocalBuild+test_copy_python_template platform-aware
…ror substring match

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…e test infra failures

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…est skip

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…55 dir

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…oken tunnel test, restore gitattributes

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
allisonschiang and others added 3 commits May 19, 2026 16:10
…evert noisy copy_local close

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels May 19, 2026
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels May 20, 2026
@@ -0,0 +1 @@
*.md text eol=lf
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Windows GitHub Actions runners default to core.autocrlf=true, which converts LF → CRLF on checkout. CRLF adds 1 byte per line. So a 167-char file checked out on Linux becomes ~171 chars on Windows, and the test fails the byte-count assertions

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

windows test will fail without it

Comment thread cli/client_test.go
filePath := utils.ResolveFile(dataFileName)
_, err = os.ReadFile(filePath)
test.That(t, err, test.ShouldBeError, fmt.Errorf("open %s: no such file or directory", filePath))
test.That(t, errors.Is(err, fs.ErrNotExist), test.ShouldBeTrue)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

windows has slightly diff error string, so just check errNotExist instead of specific string

Comment thread cli/module_registry.go
if err != nil {
return errors.Wrapf(err, "failed to create %s", manifestPath)
}
defer func() { vutils.UncheckedError(manifestFile.Close()) }()
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

on windows, all open files need to be explicitly closed

filepath: "non_existent_file.md",
shouldContain: []string{},
shouldErrorWith: "failed to read markdown file at non_existent_file.md: open non_existent_file.md: no such file or directory",
shouldErrorWith: "failed to read markdown file at non_existent_file.md: open non_existent_file.md:",
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

windows different error string

Comment thread cli/module_reload_test.go
expectedName := "viam-labs_test-module_from_reload"
expectedVersion := "latest-with-prerelease"
remoteReloadPath := ".viam/packages-local/viam-labs_test-module_from_reload-module.tar.gz"
expectedEntrypoint, err := filepath.Abs(manifest.Entrypoint)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

forward slash issues on windows

Comment thread cli/xacro.go
err = closeErr
}
}()
if err := f.Close(); err != nil {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

need to close before removing

Comment thread cli/client_test.go
ac.baseURL, _, err = utils.ParseBaseURL(ac.conf.BaseURL, false)
test.That(t, err, test.ShouldBeNil)

ac.dialOverride = func(ctx context.Context, fqdn string, rpcOpts []rpc.DialOption, logger logging.Logger) (*client.RobotClient, error) {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

TestShellFileCopy/from/single_file (540.05s) hangs with

rpc/dial.go:144 trying mDNS {"address":"99823048-4cba-4b40-823e-a26f7362d4a3"}
rpc/server.go:550 mDNS setup failed; continuing with mDNS disabled {"error":"no positive MTU found"}

by adding this, we can skip the mDNS setup that will likely fail on windows and go straight to direct grpc with the actual robot data from the test

Comment thread cli/module_build_test.go
// the manifest contains a:
// "setup": "./setup.sh"
// and a "build": "make build"
manifestPath := createTestManifest(t, "", nil)
Copy link
Copy Markdown
Member Author

@allisonschiang allisonschiang May 20, 2026

Choose a reason for hiding this comment

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

default test manifest is set up for mac, need windows specifics like .bat instead of .sh

@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels May 20, 2026
@allisonschiang allisonschiang marked this pull request as ready for review May 20, 2026 18:52
@allisonschiang allisonschiang requested review from a team, lia-viam and njooma and removed request for a team May 20, 2026 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

safe to test This pull request is marked safe to test from a trusted zone

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants