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.

Sign up for GitHub

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

Jump to bottom

Getting MDX tests working on Windows #589

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

Getting MDX tests working on Windows #589

wants to merge 2 commits into from

Conversation

polytypic
Copy link
Contributor

No description provided.

polytypic
polytypic commented Jul 21, 2023
View reviewed changes
lib_eio_windows/dune Outdated Show resolved Hide resolved
polytypic
polytypic commented Jul 21, 2023
View reviewed changes
lib_eio_windows/dune Outdated Show resolved Hide resolved
@polytypic polytypic force-pushed the fix-mdx-tests-to-run-on-windows branch from 502d91d to ad9e39c Compare July 21, 2023 11:49
polytypic
polytypic commented Jul 21, 2023
View reviewed changes
README.md

<!-- $MDX skip -->
```ocaml
# Eio_main.run @@ fun env ->
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason this test never completes. So, I disabled it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That seems worrying! Is it only an MDX thing, or does the example in examples/net also hang if the server side isn't running?

@polytypic polytypic changed the title WIP: Getting MDX tests working on Windows Getting MDX tests working on Windows Jul 21, 2023
@polytypic polytypic force-pushed the fix-mdx-tests-to-run-on-windows branch 5 times, most recently from 56d9fe9 to cfbb46b Compare July 24, 2023 08:59
polytypic
polytypic commented Jul 24, 2023
View reviewed changes
README.md Outdated Show resolved Hide resolved
@polytypic polytypic force-pushed the fix-mdx-tests-to-run-on-windows branch from cfbb46b to 988e85a Compare July 24, 2023 09:45
polytypic
polytypic commented Jul 24, 2023
View reviewed changes
README.md
@@ -904,6 +912,7 @@ ONE TWO THREE
If you want to capture the output of a process, you can provide a suitable `Eio.Flow.sink` as the `stdout` argument,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disabled the process tests. There are two problems:

  1. Process operations are not yet implemented on Windows.
  2. Even if the process operations were implemented on Windows, the tests would not work on Windows, because they use commands that do not necessarily exist on Windows.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah. We really need a way to selectively disable tests on different platforms (e.g. realworldocaml/mdx#393).

@polytypic
Copy link
Contributor Author

polytypic commented Jul 25, 2023 •
edited
Loading

We could consider adding some TODO notes to things disabled in this PR currently, merge it — so we'd have more Windows tests running on CI — and then continue with the TODOs in separate PRs. Or we can continue work in this PR and not merge it yet. Either way is fine with me.

@polytypic polytypic force-pushed the fix-mdx-tests-to-run-on-windows branch 2 times, most recently from dde2046 to 5cca345 Compare July 27, 2023 11:54
@polytypic polytypic mentioned this pull request Jul 27, 2023
Merged
@polytypic polytypic force-pushed the fix-mdx-tests-to-run-on-windows branch from 5cca345 to 2b8cded Compare July 27, 2023 15:12
@talex5 talex5 mentioned this pull request Aug 11, 2023
Merged
@polytypic polytypic force-pushed the fix-mdx-tests-to-run-on-windows branch from 2b8cded to 9b86081 Compare August 11, 2023 13:11
@polytypic polytypic force-pushed the fix-mdx-tests-to-run-on-windows branch from 9b86081 to a383832 Compare August 21, 2023 06:28
@polytypic polytypic force-pushed the fix-mdx-tests-to-run-on-windows branch 2 times, most recently from 5897e55 to 65a4936 Compare September 29, 2023 12:34
@polytypic polytypic marked this pull request as ready for review September 29, 2023 12:47
@polytypic polytypic force-pushed the fix-mdx-tests-to-run-on-windows branch from 65a4936 to 8ce161d Compare October 9, 2023 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@talex5 talex5 talex5 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@polytypic @talex5