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

Fix flaky windows test #4406

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

Fix flaky windows test #4406

wants to merge 2 commits into from

Conversation

nickva
Copy link
Contributor

@nickva nickva commented Jan 26, 2023

This PR bypasses the socket tests on windows and fixes a flaky chttpd changes tests where we made a too strong of an assumption about the order in which the shards would be returned.

Thanks to @big-r81 for investigating both issues!

See #4401 (comment) for his windows test failure report.

@nickva nickva requested a review from big-r81 January 26, 2023 05:15
@big-r81
Copy link
Contributor

big-r81 commented Jan 26, 2023

Runs through...
See log.

On Windows it seems the sockets are closed and we get the `connection_closed`
error. Account for that error as well as a 400 or a `request_failed` response.

It behaves differently on Windows. At some point in the future investigate the
details but for now let's just bypass it.
We made too strong of an assumption there that even in the case of Q=8 we'd
always have 2 pending changes, which is incorrect. The tests on Windows
apprently revealed an error where it returned 0. So let's relax the assumption
to assert that pending would be >= 0 and something less than 7.
@nickva nickva force-pushed the fix-flaky-windows-test branch from c2498a9 to 86d7ee6 Compare January 26, 2023 16:19
@big-r81
Copy link
Contributor

big-r81 commented Jan 26, 2023

Running

.\make.cmd eunit apps=chttpd suites=chttpd_socket_buffer_size_test,chttpd_changes_test

Collections
===========

     Total    Fixture       Test      Count     Failed     Errors    Skipped
     11.9s      10.5s       1.4s         40          0          0          0 EUnit        
      0.0s       0.0s       0.0s          0          0          0          0 EXUnit        
      0.0s       0.0s       0.0s          0          0          0          0 JavaScript        
      0.0s       0.0s       0.0s          0          0          0          0 Mango        

Suites
======

     Total    Fixture       Test      Count     Failed     Errors    Skipped
     11.9s      10.5s       1.4s         38          0          0          0 EUnit - chttpd_changes_test
      0.0s       0.0s       0.0s          2          0          0          0 EUnit - chttpd_socket_buffer_size_test

@big-r81
Copy link
Contributor

big-r81 commented Jan 26, 2023

Would add one additional fixture setup failure...

fixture setup [39,1,1,6]
------------------------

::in function chttpd_security_tests:create_db/1 (test/eunit/chttpd_security_tests.erl, line 60)
in call from chttpd_security_tests:setup/0 (test/eunit/chttpd_security_tests.erl, line 47)
**error:{badmatch,{error,connection_closed}}

@nickva
Copy link
Contributor Author

nickva commented Jan 26, 2023

::in function chttpd_security_tests:create_db/1 (test/eunit/chttpd_security_tests.erl, line 60)
in call from chttpd_security_tests:setup/0 (test/eunit/chttpd_security_tests.erl, line 47)
**error:{badmatch,{error,connection_closed}}

Hmm, that one is unexpected. We're just trying to create a db with a PUT and we should expect that to always work:

{ok, Status, _, _} = test_request:put(Url, [?CONTENT_JSON, ?AUTH], "{}"),

With the small buffer socket test, I can see how connection might be closed suddenly or in a different way on Windows but the simple doc PUT in a security test setup points to something else going on.

Could we be running out of sockets? Not sure descriptor limit a thing on Windows?. Try running just that one security test separately, wonder if that would fail.

Another idea is that there is a genuine race or bug somewhere in couch/ibrowse/mochiweb/erlang vm code that is exposed on Windows only.

@big-r81
Copy link
Contributor

big-r81 commented Oct 24, 2023

Moved into #4398 ...

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.

2 participants