search

agnivade / wasmbrowsertest

Run WASM tests inside your browser
MIT License
191 stars 22 forks source link

initial work for fs api forwarding #49

Closed mlctrez closed 1 year ago

mlctrez commented 1 year ago

This is the initial work for supporting filesystem api calls from wasm to write build output, test output, and go tooling output.

I've lightly tested this and there will be more unit tests for the handler to make sure the correct error codes ENOVAL, etc are returned.

mlctrez commented 1 year ago

Ugh.. CI build kicked of on my fork and discovered all of the inconsistencies in syscall.Stat_t between linux and macos. Need to re-think this a bit for GOOS!=linux

saving this as an option https://github.com/tonistiigi/fsutil/pull/71/files

agnivade commented 1 year ago

Thanks for all the work! Let me know when it's ready to review.

mlctrez commented 1 year ago

I did some initial testing on windows and ran across an issue with the GOCOVERDIR environment variable.

When running the go tooling on linux and darwin, this environment is always an absolute path starting with /.

When running the go tooling on windows, this environment is still an absolute path, but it begins with a drive letter, like C:\somepath\etc....

Since the binary under test is js/wasm, the path for os.Create triggers a check at the following location:

https://github.com/golang/go/blob/05d4b57c9fd082c8f68f454d1bb4ae26a3c7f5b9/src/syscall/fs_js.go#L104-L106

This results in os.Create() failing due to cwd not being implemented.

It might be possible to modify the environment passed down to the js/wasm test binary and trick it into thinking the path is unix like. But the open calls in wasmbrowser test running as a windows binary would need to check for this and modify the path(s) back to include the drive letter.

agnivade commented 1 year ago

Is everything working fine on linux and darwin? Happy to open up initial support only for those platforms.

mlctrez commented 1 year ago

Did some more work on the windows implementation and it's tested and working as far as I can tell. I've squashed the commits in this branch to make it a bit easier to review.

As for linux / darwin testing, I've tested mainly on linux but I believe the differences between linux and darwin to be minor enough that it should be OK. It is just some struct field name differences and that's handled with specific files for each OS.

mlctrez commented 1 year ago

I've been adding some more test coverage, might be broken for a bit

mlctrez commented 1 year ago

The tests have been cleaned up and simplified quite a bit. Latest CI build shows green.

@agnivade - This should be ready for a real review now.

agnivade commented 1 year ago

@mlctrez - Looks like there's some problem with CI:

Download action repository 'actions/setup-go@v4' (SHA:93397bea11091df50f3d7e59dc26a7711a8bcfbe)
Download action repository 'actions/checkout@v3' (SHA:f43a0e5ff2bd294095638e18286ca9a3d1956744)
Error: Can't use 'tar -xzf' extract archive file: /home/runner/work/_actions/_temp_aa91c19b-fdeb-4e4b-a4eb-f564cac74fc5/bedbff20-65da-447e-8888-e0ece46c0da1.tar.gz. return code: 2.

Perhaps bumping up the versions of actions/checkout and actions/setup-go has something to do with it?

mlctrez commented 1 year ago

I bumped those up due to deprecation warnings with the current action versions when running CI on my fork. I've backed out that change to see if it fixes your CI build.

mlctrez commented 1 year ago

It looks like there is an event with the github system right now. May be related: https://www.githubstatus.com/history

image

agnivade commented 1 year ago

Ah good spot. I'll send a PR separately to bump those. Let's keep this one focused to only fs changes.

agnivade commented 1 year ago

@mlctrez - Apologies for the delay here. I am trying to find some time to review this.

mlctrez commented 1 year ago

@agnivade No rush, I've been testing using my forked code.

mlctrez commented 1 year ago

@agnivade I replied to your comments. I'll push my changes from your review to this branch as a new commit, unless you would prefer to squash the changes on top of the original. That can always be done later I guess.

Edit: The CI build for this branch failed on the first attempt but succeeded on the next attempt when I re-ran.
I don't think this is related to my code changes since the error is a websocket error, and I don't see where that would be used in the codebase. Possibly a github glitch with actions?

agnivade commented 1 year ago

I don't think this is related to my code changes since the error is a websocket error, and I don't see where that would be used in the codebase. Possibly a github glitch with actions?

Yeah it's a flaky thing that happens on Mac/Windows. Either we need to get rid of the test or somehow improve that. Nothing related to this PR.

mlctrez commented 1 year ago

@agnivade Made most all the changes you suggested and added to conversations where changes were not made.