Run the full test suite in pure ruby CI #1006

nevans · 2024-09-11T15:00:05Z

~~Many of these may be trivial to implement in pure ruby mode, and some may be impossible, but for right now I'd prefer to simply have them all displayed as a "TODO list" in the test results.~~

~~Of course, adding these stub methods breaks many tests that were omitted based on whether or not EM responds to these methods.~~

All of the failing tests have been marked as pending, including the ones that failed when stub methods were added and raised EM::Unsupported. Where the tests failed due to an EM::Unsupported error, the name of the stubbed method has been added to the pending message. This should simplify finding tests to un-pend, if and when pure ruby mode is fixed to support these features.

Additionally, CI for pure ruby mode has been updated to run the entire test suite--albeit with 90 tests marked as pending. This should allow us to "ratchet" pure ruby mode forward.

~~The major downside~~ ~~A minor tradeoff of this PR is that EM.respond_to?(method) is no longer a reliable feature detection mechanism. (see the comment below for an analysis of this issue)~~ This has been moved into another PR.

nevans · 2024-09-11T15:08:18Z

@sodabrew @MSP-Greg I'd totally forgotten that rake test_em_pure_ruby only ran a subset of the test suite. Although I had to mark an enormous number of tests as pending, it also adds an enormous number of passing assertions to CI.

In the main branch:

Finished in 7.218512774 seconds.
-------------------------------------------------------------------------------
56 tests, 124 assertions, 0 failures, 0 errors, 0 pendings, 12 omissions, 0 notifications
100% passed
-------------------------------------------------------------------------------
7.76 tests/s, 17.18 assertions/s

In this PR:

Finished in 31.2188496 seconds.
-------------------------------------------------------------------------------
310 tests, 3479 assertions, 0 failures, 0 errors, 90 pendings, 14 omissions, 0 notifications
69.5946% passed
-------------------------------------------------------------------------------
9.93 tests/s, 111.44 assertions/s

nevans · 2024-09-11T15:17:39Z

Treating pend as "to do" and omit as "can't/won't fix", some of these pend may need to be converted to omit, and vice versa. I haven't looked very closely at each of the methods.

My immediate goal isn't to work through that TODO list, it's just to make sure the "ratchet" is in place so we don't regress.

My longer term goal is to get my existing EventMachine services either migrated to a modern gem, or to modernize EventMachine enough that I don't care any more. 😉 I figure that both goals are aided by getting more of the pure ruby implementation working. In my opinion, much of the C++ code can and should eventually be converted to ruby, except where is is significant for performance of course.

sodabrew · 2024-09-11T17:46:01Z

This look fantastic. I'd like to merge it as-is, just note if you think you're still working on it or ready to land!

nevans · 2024-09-11T18:42:32Z

This look fantastic. I'd like to merge it as-is, just note if you think you're still working on it or ready to land!

@sodabrew Thanks! Yeah, for this PR I'm not going to worry about omit vs pend. It's good to go from my end.

sodabrew · 2024-09-11T18:51:37Z

... thinking some more:

The major downside of this PR is that EM.respond_to?(method) is no longer a reliable feature detection mechanism.

I'm using GitHub and GitLab code search to see if this is a common pattern, I did find a few:
https://github.com/search?q=%22%20EM.respond_to%3F%22++path%3A*.rb&type=Code&ref=advsearch&l=&l=

nevans · 2024-09-11T19:44:31Z

... thinking some more:

The major downside of this PR is that EM.respond_to?(method) is no longer a reliable feature detection mechanism.

I'm using GitHub and GitLab code search to see if this is a common pattern, I did find a few: https://github.com/search?q=%22%20EM.respond_to%3F%22++path%3A*.rb&type=Code&ref=advsearch&l=&l=

Thanks for taking that next step. Yeah, this is the potential issue.

So I looked at all of the examples, and also searched for EventMachine.respond_to?, and I think we're okay. I'm fairly certain all of the search results are one of the following:

Many results are simply from eventmachine/eventmachine or forks: they are simply the respond_to? calls in our own test suite.
Several repos have vendored copies of eventmachine, so the same deal there as the forks.
Most of the (not forked or bundled) repos were checking for a "public" (and documented as public) method that isn't stubbed or changed by this PR. These are basically using EM.respond_to? to feature test for the version of EventMachine, not for the implementation mode. For example: watch, reactor_running?, reactor_thread, attach, defers_finished?, tick_loop,
In at least two repos, they were guarding against their own alias method chaining (thus unchanged by this PR).
In at least one repo, they were checking for a method added by themselves or by a different gem (thus unchanged by this PR).
In at least one repo, it was em.respond_to? for an abbreviated "embed" argument (i.e: not EventMachine).

But there's a deeper reason I think we're okay: pretty close to no one should be using pure ruby mode for very much today... or if they are, they're dealing with lots and lots of bugs already (whether they known it or not), and that's regardless of whether or not they are using respond_to? for feature detection. Because there are still some basic pieces of broken functionality in pure ruby mode! This PR only really breaks respond_to? if you enable pure ruby mode, but that approach is already broken.

Additionally, most of the stubbed methods are either undocumented or documented as @private, and many of them require a signature parameter or return a signature. Likewise, most of them have another related method that is documented and doesn't work with internal event descriptor signatures. So, although I wish these quasi-private methods were handled differently, it looks like users of EventMachine are mostly ignoring them and not making dependencies on them. That's good... both for this PR and for others I'll propose in the future. 😉

The tests have been updated with "pend" or "omit", so the test suite should still pass (albeit with a long "TODO list" in the output).

There's no longer any special reason to single out just these tests, so this rake task is obsolete.

nevans · 2024-09-12T17:00:44Z

@sodabrew I still think it's safe for the reasons given, but its not completely risk-free, so I'm also fine with putting that part off for a future PR. I've removed almost all of the stubs. The only stub methods this PR adds now are the ones related to epoll and kqueue, so rake test doesn't immediately crash.

There are still many files that check EM.respond_to?(method) before even defining certain test case classes or methods, and those won't be added to the "pending" totals. Even without those, this is still a huge improvement. Compare to the numbers in #1006 (comment):

Finished in 31.088036539 seconds.
-------------------------------------------------------------------------------
301 tests, 3482 assertions, 0 failures, 0 errors, 76 pendings, 14 omissions, 0 notifications
73.5192% passed
-------------------------------------------------------------------------------
9.68 tests/s, 112.00 assertions/s

(Somewhat ironically, the test suite seems to make more assertions when the methods are missing.)

This stubbing is just enough to get the rest of the test suite running. It should be safe, as `EM.kqueue?` and `EM.epoll?` already exist for feature detection. The implementation here basically mimics the implementation in ext/rubymain.cpp.

nevans changed the title ~~Mark pure ruby tests pending~~ Add pure ruby stub methods and run all tests in pure ruby mode Sep 11, 2024

nevans changed the title ~~Add pure ruby stub methods and run all tests in pure ruby mode~~ Add pure ruby stub methods and run all tests in pure ruby CI Sep 11, 2024

nevans added 3 commits September 12, 2024 12:36

Mark failing tests as pending for pure ruby mode

7df8ecd

Change test_em_pure_ruby to execute *all* tests

fbec14b

The tests have been updated with "pend" or "omit", so the test suite should still pass (albeit with a long "TODO list" in the output).

Drop rake test_pure

9492197

There's no longer any special reason to single out just these tests, so this rake task is obsolete.

nevans force-pushed the mark-pure-ruby-tests-pending branch from 53847f9 to dfb76cc Compare September 12, 2024 16:50

nevans changed the title ~~Add pure ruby stub methods and run all tests in pure ruby CI~~ Run the full test suite in pure ruby CI Sep 12, 2024

nevans mentioned this pull request Sep 12, 2024

Add pure ruby stub methods (raise EM::Unsupported) #1008

Open

nevans force-pushed the mark-pure-ruby-tests-pending branch from dfb76cc to a6cbdf1 Compare September 12, 2024 17:17

sodabrew merged commit 02ca4fe into eventmachine:master Sep 16, 2024
44 checks passed

nevans deleted the mark-pure-ruby-tests-pending branch September 18, 2024 21:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Run the full test suite in pure ruby CI #1006

Run the full test suite in pure ruby CI #1006

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Run the full test suite in pure ruby CI #1006

Run the full test suite in pure ruby CI #1006

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!