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

Added skip #778

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Added skip #778

wants to merge 6 commits into from

Conversation

create2000
Copy link
Contributor

This is a work-in-progress that adds skip to the test file.

@create2000
Copy link
Contributor Author

create2000 commented Oct 29, 2024

Hello @patricoferris and @talex5 -- this is the new PR.

The questions asked here are still much needed here too. Thank you :))

Copy link
Collaborator

@talex5 talex5 left a comment

Choose a reason for hiding this comment

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

You need to define the skip function too (to raise a suitable exception for MDX).

BTW, you don't need to open a new PR for every revision. Just update your existing branch (git push -f to force-update if you're undoing old commits).


let test_poll_add_busy () =
Eio_linux.run ~queue_depth:2 @@ fun _stdenv ->
Eio_linux.run ~fallback:skip (fun _stdenv ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

You still need the queue depth. And there's no need to remove the @@.

Copy link
Contributor Author

@create2000 create2000 Nov 3, 2024

Choose a reason for hiding this comment

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

Thank you @talex5 --

I have made some changes.

@create2000 create2000 marked this pull request as ready for review November 8, 2024 10:01
@create2000 create2000 changed the title WIP: added skip Added skip Nov 8, 2024
Copy link
Collaborator

@patricoferris patricoferris left a comment

Choose a reason for hiding this comment

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

For skipping tests in the Alcotest framework there is https://ocaml.org/p/alcotest/1.8.0/doc/Alcotest/index.html#val-skip

@@ -19,7 +23,7 @@ let read_one_byte ~sw r =
)

let test_poll_add () =
Eio_linux.run @@ fun _stdenv ->
Eio_linux.run ~fallback:skip (fun _stdenv ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

Still no need to make the change from @@ to (

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @patricoferris -- I have gone through the documentation, To get it correct, I used Alcotest.skip. Something like this

let skip_io_uring () = 
    Alcotest.skip "Skipping test: io_uring not available in Docker";;

However, when i use this, i encounter this error: Alcotest.skip "Skipping test: io_uring not available in Docker";; Error: This expression has type string but an expression was expected of type unit

Is there something i am doing wrong? I would appreciate help.

Copy link
Collaborator

@patricoferris patricoferris Nov 15, 2024

Choose a reason for hiding this comment

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

@create2000 the type of skip is

val skip : unit -> 'a 

It doesn't expect a string as the first argument but a value of type unit (i.e ()). You could perhaps log something with Eio.traceln and then Alcotest.skip (). Note that this is only for the Alcotest-based Linux tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@patricoferris Thank you for the clarification. I have modified it and logged a message using the Eio.tracein which was displayed in the generated file after running the file.

This is the new modification:

let skip_io_uring () =
  Eio.traceln "Skipping test: io_uring not available in Docker";
  Alcotest.skip ()
let test_poll_add () =
  Eio_linux.run ~fallback:(skip_io_uring()) 

Copy link
Collaborator

@talex5 talex5 left a comment

Choose a reason for hiding this comment

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

Note that in #777 you were trying to make this work for MDX (.md) files, but this PR is changing an Alcotest (.ml) file. Here you don't need a new exception, as Alcotest already supports skipping. However, you should use the error message that Eio_linux.run gives you, rather than just assuming that the problem is Docker.

@create2000
Copy link
Contributor Author

create2000 commented Nov 16, 2024

Hello @talex5 -- thank you for the insight. How do I get the error message from Eio_linux.run?

@talex5
Copy link
Collaborator

talex5 commented Nov 17, 2024

The argument to fallback is [`Msg of string].

@create2000
Copy link
Contributor Author

Hello @talex5 -- regarding adding the skip feature to MDX, I discovered there are two .md files: spawn.md and fd_sharing.md. Am I to create a new .md file or add the skip feature to one of the already existing .md files?

patricoferris and others added 5 commits November 20, 2024 16:05
Symlinking is privileged operation on Windows, so we check if the running user can make symlinks before running tests that require them.
If the fiber fails, record the backtrace when failing the switch. `fork`
itself already did this, but `fork_daemon` and `fork_promise_exn`
didn't.
@talex5
Copy link
Collaborator

talex5 commented Nov 22, 2024

The skip feature needs adding to MDX itself (not Eio). But existing md files will need to be modified to use it.

@create2000
Copy link
Contributor Author

@talex5 -- I have been trying to add the skip feature to MDX and I need some help:

First, I have MDX installed via spam, but when I try to check the version, it doesn't show. So, I decided to clone the MDX repo and followed the instructions in the contributing.md file. Now, when i run the test, it shows me the output. But how do i proceed to create the skip feature?

From the contributing.md I'll have to use the test\lib for unit tests and the test\bin for end-to-end test. This is where i am confused. Do i need to create a separate file in the test\lib for my skip feature? And will i need to further add it to my test\bin?

I will appreciate any additional help. Thank you ;))

@talex5
Copy link
Collaborator

talex5 commented Nov 26, 2024

You'll need to add a test to test/bin I would expect.

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.

3 participants