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

Adding new tests to libzim #67

Merged
merged 6 commits into from
Dec 6, 2023
Merged

Adding new tests to libzim #67

merged 6 commits into from
Dec 6, 2023

Conversation

Rishabhg71
Copy link
Collaborator

I have added the -s DISABLE_EXCEPTION_CATCHING=0 to compile the files properly.

@Rishabhg71
Copy link
Collaborator Author

is it okay to let the GitHub actions run? I am seeing some steps where files may be updated on server

@Rishabhg71
Copy link
Collaborator Author

@Jaifroid Tests are now enabled on all the build files + could you answer my question above?

@Jaifroid
Copy link
Collaborator

Jaifroid commented Dec 2, 2023

@Rishabhg71 I'm sorry, I didn't see the notifications from your previous message and query. You can run GitHub actions. "Build and publish release artefacts" (https://github.com/openzim/javascript-libzim/actions/workflows/build_libzim_wasm.yml) explains what it does in the Run Workflow dropdown. I you run it as "nightly", then it will create artefacts and upload them to the nightly server. If you run it as "release" but without a version number, then it will just build artefacts and archive them under the workflow summary. If you add a version number, then it will create a draft release in releases (which can simply be deleted if it's a test run).

"Upload release assets to Kiwix" only runs after a draft release is published. So no need to worry about it if you're not actually pressing the "Publish" button on a draft release!

@Jaifroid
Copy link
Collaborator

Jaifroid commented Dec 2, 2023

Forgot to add that automatic running of workflows when changes to a PR are pushed only produce archived versions under the workflow, they don't publish them to the Kiwix server, so it's safe to let the workflow complete after push, or to re-run it.

@Jaifroid Jaifroid added build Code relating to building or publishing assets tests labels Dec 2, 2023
@Rishabhg71
Copy link
Collaborator Author

A difference between the production and the debug builds is that the latter have -s DISABLE_EXCEPTION_CATCHING=0 in the compile command. For more info, see https://emscripten.org/docs/porting/exceptions.html#emscripten-javascript-based-exception-support .
Rather than compiling the production versions with enabled exception catching, we should make use of https://github.com/emscripten-core/emscripten/blob/main/src/settings.js#L683 (EXCEPTION_CATCHING_ALLOWED = [ ]) to enable catching only in the affected function(s).

How do I know which functions should be passed as a parameter? I think getEntryByPath is the only function or am I wrong?

@Jaifroid
Copy link
Collaborator

Jaifroid commented Dec 2, 2023

@Rishabhg71 The functions for which we have written bindings are the articleCount, search, and getDirectoryByPath. We need to write some more, but they're not urgent, because we handle everything else in the backend ourselves currently,

@Rishabhg71
Copy link
Collaborator Author

@Jaifroid I have added -s EXCEPTION_CATCHING_ALLOWED=["getEntryByPath","search","getArticleCount","loadArchive"] but seems like I still that uncaught error. I am not sure why this is happening any idea? building with -s DISABLE_EXCEPTION_CATCHING=0 is working fine
image

@Jaifroid
Copy link
Collaborator

Jaifroid commented Dec 5, 2023

@Rishabhg71 I'm afraid I don't know, and I've never used that setting allowing exception catching for individual functions. I would say that if the size of the bundle is not significantly increased by the general setting, it's probably safe to use it, unless you can find some info on how to get the narrower exception catching working...

@Rishabhg71
Copy link
Collaborator Author

Rishabhg71 commented Dec 5, 2023

I think the size is good enough it has only increased 20 kb max, files with suffix "1" are compiled byDISABLE_EXCEPTION_CATCHING. I have limited experience, I did some googling and prompting but didn't get any further
image

If this is acceptable I think then you can do a first review now

@Jaifroid
Copy link
Collaborator

Jaifroid commented Dec 5, 2023

OK, thanks, I'll review it, but in the meantime, you might want to revert the individual exception catching, which is still in the PR and is not working, right? Certainly the tests have failed, not 100% sure if it¡'s for that reason.

Copy link
Collaborator

@Jaifroid Jaifroid left a comment

Choose a reason for hiding this comment

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

It's all looking good to me -- well written code! Just the few queries below, and I'd like to see a run passing with both Ray Charles and Stackexchange being tested.

tests/prototype/chromium.e2e.runner.js Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
@Rishabhg71
Copy link
Collaborator Author

@Jaifroid Sorry I forgot to revert my changes, you can check again

Copy link
Collaborator

@Jaifroid Jaifroid left a comment

Choose a reason for hiding this comment

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

Looks very good to me, with just the two small changes below! After this is merged, we should create a new release, so that we can use officially released artefacts over at Kiwix JS.

tests/prototype/stackexchange.e2e.spec.js Outdated Show resolved Hide resolved
tests/prototype/ray_charles.e2e.spec.js Show resolved Hide resolved
@Rishabhg71 Rishabhg71 merged commit b0d65e3 into main Dec 6, 2023
2 checks passed
@Rishabhg71 Rishabhg71 deleted the libzim-new-tests branch December 6, 2023 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Code relating to building or publishing assets tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Builds from 0.0.2 onwards (probably also 0.0.1) appear to stall if a dirEntry is not found
2 participants