Skip to content

Test reading from an offset with the opfs backend.#19139

Open
brendandahl wants to merge 1 commit intoemscripten-core:mainfrom
brendandahl:opfs-seek-test
Open

Test reading from an offset with the opfs backend.#19139
brendandahl wants to merge 1 commit intoemscripten-core:mainfrom
brendandahl:opfs-seek-test

Conversation

@brendandahl
Copy link
Collaborator

I noticed in the JSPI work that this code path wasn't being exercised in the tests.

I noticed in the JSPI work that this code path wasn't being exercised
in the tests.
nread = read(fd, buf4, 4);
assert(nread == 4);
assert(strcmp(buf4, "ello") == 0);
emscripten_console_logf("read message: %s (%d)", buf4, nread);
Copy link
Collaborator

@sbc100 sbc100 Apr 12, 2023

Choose a reason for hiding this comment

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

Unrelated to this test, but every time I see changes like this I wonder why we have tests like this? Isn't this test just a generic file I/O test that should work with any backend not just OPFS? Same for wasmfs vs legacy FS. Can't we just have one set of the file I/O tests that work on each of the wasmfs backends and on the legacy FS.

I worry that we have many duplicate and overlapping tests at this point and consolidation is going to be hard (or just never happen).

I know I keep harping on about this but I worry we are moving in the wrong direction.

(@tlively @azakai)

Copy link
Member

Choose a reason for hiding this comment

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

Writing new tests was unavoidable at first, but now WasmFS should be feature complete that in principle we could consolidate tests, at least for some things. It's just that it already would be a lot of work :(

Copy link
Member

Choose a reason for hiding this comment

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

I agree we should prioritize this now. How about adding no further new tests, and trying to get to this test consolidation before any other significant work on WasmFS?

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.

4 participants