You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by "jiahuili430 (via GitHub)" <gi...@apache.org> on 2025/10/14 19:57:24 UTC

[PR] Fix flaky tests by adding `Process.sleep(100)` [couchdb]

jiahuili430 opened a new pull request, #5702:
URL: https://github.com/apache/couchdb/pull/5702

   <!-- Thank you for your contribution!
   
        Please file this form by replacing the Markdown comments
        with your text. If a section needs no action - remove it.
   
        Also remember, that CouchDB uses the Review-Then-Commit (RTC) model
        of code collaboration. Positive feedback is represented +1 from committers
        and negative is a -1. The -1 also means veto, and needs to be addressed
        to proceed. Once there are no objections, the PR can be merged by a
        CouchDB committer.
   
        See: http://couchdb.apache.org/bylaws.html#decisions for more info. -->
   
   ## Overview
   
   <!-- Please give a short brief for the pull request,
        what problem it solves or how it makes things better. -->
   
   When running elixir tests with `OTP 27` and `Elixir 1.18.4`, `changes_async_test.exs` became flaky. Adding `IO.inspect/2` fixed the test, which indicate some race conditions.
   
   Therefore, adding `Process.sleep(100)` to get the entire response from the `_change` endpoint and then verify it.
   
   
   ## Testing recommendations
   
   <!-- Describe how we can test your changes.
        Does it provide any behaviour that the end users
        could notice? -->
   
   ## Related Issues or Pull Requests
   
   <!-- If your changes affect multiple components in different
        repositories please put links to those issues or pull requests here.  -->
   
   ## Checklist
   
   - [ ] Code is written and works correctly
   - [ ] Changes are covered by tests
   - [ ] Any new configurable parameters are documented in `rel/overlay/etc/default.ini`
   - [ ] Documentation changes were made in the `src/docs` folder
   - [ ] Documentation changes were backported (separated PR) to affected branches
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Add `retry_until` for flaky `changes_async_test.exs` tests [couchdb]

Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5702:
URL: https://github.com/apache/couchdb/pull/5702#discussion_r2430596629


##########
test/elixir/test/changes_async_test.exs:
##########
@@ -95,6 +95,7 @@ defmodule ChangesAsyncTest do
     create_doc_bar(db_name, "bar")
 
     changes = process_response(req_id.id, &parse_event/1)
+    Process.sleep(100)

Review Comment:
   Yeah it's curious how it works. Maybe adding some extra debug logs around parsing the result could reveal what's happening.
   
   I am not very familiar with Elixir, does it look like `changes` is pulling data lazily when we call `length()` the first  time? Otherwise the way it looks is after `changes = ...` it shouldn't matter if we `sleep`, change is already fetched (either completely or partially).



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Increase timeout for `process_response/3` to fix flaky tests [couchdb]

Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5702:
URL: https://github.com/apache/couchdb/pull/5702#discussion_r2440960339


##########
src/couch_replicator/test/eunit/couch_replicator_test_helper.erl:
##########
@@ -206,6 +206,7 @@ test_setup() ->
     Hashed = couch_passwords:hash_admin_password(?PASSWORD),
     Persist = false,
     ok = config:set("admins", ?USERNAME, ?b2l(Hashed), Persist),
+    ok = config:set("replicator", "use_bulk_get", "true", Persist),

Review Comment:
   In this case we'd probably just want to reset it back after the test is done. It will be a semantic change, to look cleaner (we did some thing, we undid it when done), but in reality since we're using persist false and tests start a new config application instance, it shouldn't make a practical difference. This can be a separate PR or just punt on it.
   
   > I'll update the default.ini file
   
   I don't think we need  to update default.ini?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Add `retry_until` for flaky `changes_async_test.exs` tests [couchdb]

Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5702:
URL: https://github.com/apache/couchdb/pull/5702#discussion_r2436824969


##########
src/couch_replicator/test/eunit/couch_replicator_test_helper.erl:
##########
@@ -206,6 +206,7 @@ test_setup() ->
     Hashed = couch_passwords:hash_admin_password(?PASSWORD),
     Persist = false,
     ok = config:set("admins", ?USERNAME, ?b2l(Hashed), Persist),
+    ok = config:set("replicator", "use_bulk_get", "true", Persist),

Review Comment:
   But tests pass as is without any changes already.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Increase timeout for `process_response/3` to fix flaky tests [couchdb]

Posted by "jiahuili430 (via GitHub)" <gi...@apache.org>.
jiahuili430 merged PR #5702:
URL: https://github.com/apache/couchdb/pull/5702


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Add `retry_until` for flaky `changes_async_test.exs` tests [couchdb]

Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5702:
URL: https://github.com/apache/couchdb/pull/5702#discussion_r2433809207


##########
src/couch_replicator/test/eunit/couch_replicator_test_helper.erl:
##########
@@ -206,6 +206,7 @@ test_setup() ->
     Hashed = couch_passwords:hash_admin_password(?PASSWORD),
     Persist = false,
     ok = config:set("admins", ?USERNAME, ?b2l(Hashed), Persist),
+    ok = config:set("replicator", "use_bulk_get", "true", Persist),

Review Comment:
   use_bulk_get should already default to true or is some test leaving it disabled? in that case we should fix that test
   
   It also doesn't seem to be related to changes_sync PR in general? That's fine that it's in a separate commit but the PR message should mention both or indicate why we need it.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Fix flaky tests by adding `Process.sleep(100)` [couchdb]

Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5702:
URL: https://github.com/apache/couchdb/pull/5702#discussion_r2430375524


##########
test/elixir/test/changes_async_test.exs:
##########
@@ -95,6 +95,7 @@ defmodule ChangesAsyncTest do
     create_doc_bar(db_name, "bar")
 
     changes = process_response(req_id.id, &parse_event/1)
+    Process.sleep(100)

Review Comment:
   At first sight, it is odd that this works. By this point, it seems the `changes` are already processed since we're just checking length next. Unless `changes` is some lazy callback feeder object which only starts to produce data when we try to read the data?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Increase timeout for `process_response/3` to fix flaky tests [couchdb]

Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5702:
URL: https://github.com/apache/couchdb/pull/5702#discussion_r2440960339


##########
src/couch_replicator/test/eunit/couch_replicator_test_helper.erl:
##########
@@ -206,6 +206,7 @@ test_setup() ->
     Hashed = couch_passwords:hash_admin_password(?PASSWORD),
     Persist = false,
     ok = config:set("admins", ?USERNAME, ?b2l(Hashed), Persist),
+    ok = config:set("replicator", "use_bulk_get", "true", Persist),

Review Comment:
   In this case we'd probably just want to just want to reset it back after the test is done. It will be semantic change, to look cleaner (we did some thing, we undid it when done), but in reality since we're using persist false and tests start new config application instance, it shouldn't make a practical difference. This can be a separate PR or punt on it.
   
   > I'll update the default.ini file
   
   I don't think we need  to update default.ini?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Add `retry_until` for flaky `changes_async_test.exs` tests [couchdb]

Posted by "jiahuili430 (via GitHub)" <gi...@apache.org>.
jiahuili430 commented on code in PR #5702:
URL: https://github.com/apache/couchdb/pull/5702#discussion_r2436772089


##########
src/couch_replicator/test/eunit/couch_replicator_test_helper.erl:
##########
@@ -206,6 +206,7 @@ test_setup() ->
     Hashed = couch_passwords:hash_admin_password(?PASSWORD),
     Persist = false,
     ok = config:set("admins", ?USERNAME, ?b2l(Hashed), Persist),
+    ok = config:set("replicator", "use_bulk_get", "true", Persist),

Review Comment:
   If we had set different settings in the default.ini file, these tests would fail.
   So just trying to set it to `true` explicitly in tests.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Increase timeout for `process_response/3` to fix flaky tests [couchdb]

Posted by "jiahuili430 (via GitHub)" <gi...@apache.org>.
jiahuili430 commented on code in PR #5702:
URL: https://github.com/apache/couchdb/pull/5702#discussion_r2440983041


##########
src/couch_replicator/test/eunit/couch_replicator_test_helper.erl:
##########
@@ -206,6 +206,7 @@ test_setup() ->
     Hashed = couch_passwords:hash_admin_password(?PASSWORD),
     Persist = false,
     ok = config:set("admins", ?USERNAME, ?b2l(Hashed), Persist),
+    ok = config:set("replicator", "use_bulk_get", "true", Persist),

Review Comment:
   I mean the `default.ini` in Cloudant not CouchDB.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Increase timeout for `process_response/3` to fix flaky tests [couchdb]

Posted by "jiahuili430 (via GitHub)" <gi...@apache.org>.
jiahuili430 commented on code in PR #5702:
URL: https://github.com/apache/couchdb/pull/5702#discussion_r2440750792


##########
test/elixir/test/changes_async_test.exs:
##########
@@ -94,11 +94,13 @@ defmodule ChangesAsyncTest do
 
     create_doc_bar(db_name, "bar")
 
-    changes = process_response(req_id.id, &parse_event/1)
+    retry_until(fn ->

Review Comment:
   Increasing the timeout of `process_reposeon/3` to 3 seconds worked, thank you for the suggestion!



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Fix flaky tests by adding `Process.sleep(100)` [couchdb]

Posted by "jiahuili430 (via GitHub)" <gi...@apache.org>.
jiahuili430 commented on code in PR #5702:
URL: https://github.com/apache/couchdb/pull/5702#discussion_r2430460878


##########
test/elixir/test/changes_async_test.exs:
##########
@@ -95,6 +95,7 @@ defmodule ChangesAsyncTest do
     create_doc_bar(db_name, "bar")
 
     changes = process_response(req_id.id, &parse_event/1)
+    Process.sleep(100)

Review Comment:
   https://github.com/apache/couchdb/blob/main/test/elixir/test/changes_async_test.exs#L397
   
   Maybe latest OTP 27 or Elixir 1.18 is a little bit than OTP 26 and Elixir 1.17.
   
   It feels like it's not having enough time to process and only returns the first element. Without `IO.inspect/2`, the changes_ids will be `["doc1"]` sometimes.
   
   Maybe I should use `IO.inspect/2` as it will also introduce some sleeps and easier to debug.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Add debug lines for flaky elixir tests [couchdb]

Posted by "jiahuili430 (via GitHub)" <gi...@apache.org>.
jiahuili430 commented on code in PR #5702:
URL: https://github.com/apache/couchdb/pull/5702#discussion_r2430460878


##########
test/elixir/test/changes_async_test.exs:
##########
@@ -95,6 +95,7 @@ defmodule ChangesAsyncTest do
     create_doc_bar(db_name, "bar")
 
     changes = process_response(req_id.id, &parse_event/1)
+    Process.sleep(100)

Review Comment:
   https://github.com/apache/couchdb/blob/main/test/elixir/test/changes_async_test.exs#L397
   
   Maybe latest OTP 27 or Elixir 1.18 is a little bit slower than OTP 26 and Elixir 1.17.
   
   It feels like it's not having enough time to process and only returns the first element. Without `IO.inspect/2`, the changes_ids will be `["doc1"]` sometimes.
   
   Maybe I should use `IO.inspect/2` as it will also introduce some sleeps and easier to debug.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Increase timeout for `process_response/3` to fix flaky tests [couchdb]

Posted by "jiahuili430 (via GitHub)" <gi...@apache.org>.
jiahuili430 commented on code in PR #5702:
URL: https://github.com/apache/couchdb/pull/5702#discussion_r2440756873


##########
src/couch_replicator/test/eunit/couch_replicator_test_helper.erl:
##########
@@ -206,6 +206,7 @@ test_setup() ->
     Hashed = couch_passwords:hash_admin_password(?PASSWORD),
     Persist = false,
     ok = config:set("admins", ?USERNAME, ?b2l(Hashed), Persist),
+    ok = config:set("replicator", "use_bulk_get", "true", Persist),

Review Comment:
   I'll update the `default.ini` file and add some comments about the differences. Thanks Nick and Ronny.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Add `retry_until` for flaky `changes_async_test.exs` tests [couchdb]

Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5702:
URL: https://github.com/apache/couchdb/pull/5702#discussion_r2436878880


##########
src/couch_replicator/test/eunit/couch_replicator_test_helper.erl:
##########
@@ -206,6 +206,7 @@ test_setup() ->
     Hashed = couch_passwords:hash_admin_password(?PASSWORD),
     Persist = false,
     ok = config:set("admins", ?USERNAME, ?b2l(Hashed), Persist),
+    ok = config:set("replicator", "use_bulk_get", "true", Persist),

Review Comment:
   > and don't reset it back ...
   
   Good find, Ronny! That's a good thing to fix. However if we still set it with Persist=false it should be wiped out on next application cycle restart.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Increase timeout for `process_response/3` to fix flaky tests [couchdb]

Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5702:
URL: https://github.com/apache/couchdb/pull/5702#discussion_r2441001318


##########
src/couch_replicator/test/eunit/couch_replicator_test_helper.erl:
##########
@@ -206,6 +206,7 @@ test_setup() ->
     Hashed = couch_passwords:hash_admin_password(?PASSWORD),
     Persist = false,
     ok = config:set("admins", ?USERNAME, ?b2l(Hashed), Persist),
+    ok = config:set("replicator", "use_bulk_get", "true", Persist),

Review Comment:
   > I mean the `default.ini` in Cloudant not CouchDB.
   
   Ah, no worries, that makes sense. For Ronny's info we're trying to make the tests use the same default ini and reduce the amount of differences :-)



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Add `retry_until` for flaky `changes_async_test.exs` tests [couchdb]

Posted by "big-r81 (via GitHub)" <gi...@apache.org>.
big-r81 commented on code in PR #5702:
URL: https://github.com/apache/couchdb/pull/5702#discussion_r2436838973


##########
src/couch_replicator/test/eunit/couch_replicator_test_helper.erl:
##########
@@ -206,6 +206,7 @@ test_setup() ->
     Hashed = couch_passwords:hash_admin_password(?PASSWORD),
     Persist = false,
     ok = config:set("admins", ?USERNAME, ?b2l(Hashed), Persist),
+    ok = config:set("replicator", "use_bulk_get", "true", Persist),

Review Comment:
   > use_bulk_get should already default to true or is some test leaving it disabled? in that case we should fix that test
   
   `couch_replicator_bulk_get_tests.erl` sets it explicit to `false` in [job_enable_overrides_global_disable](https://github.com/apache/couchdb/blob/26920c21f82be1ab7297feab69e052e437fbe406/src/couch_replicator/test/eunit/couch_replicator_bulk_get_tests.erl#L112) and [global_disable_works](https://github.com/apache/couchdb/blob/26920c21f82be1ab7297feab69e052e437fbe406/src/couch_replicator/test/eunit/couch_replicator_bulk_get_tests.erl#L124), and don't reset it back ...
   
   
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Add `retry_until` for flaky `changes_async_test.exs` tests [couchdb]

Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5702:
URL: https://github.com/apache/couchdb/pull/5702#discussion_r2436913859


##########
test/elixir/test/changes_async_test.exs:
##########
@@ -94,11 +94,13 @@ defmodule ChangesAsyncTest do
 
     create_doc_bar(db_name, "bar")
 
-    changes = process_response(req_id.id, &parse_event/1)
+    retry_until(fn ->

Review Comment:
   process_response doesn't seem to be idempotent so it's seems fishy that we retry at all here.
   
   The way it seems to work is it parses body lines from the socket or gets a timeout and returns. It could be that in one run we parse one line, then timeout, but on retry that 1st line won't be refetched, as far as the socket is concerned it's already yielded the result. So on a retry we may get the second line only and still fail.
   
   It seems the original idea of sleeping a little would have worked better, however, I wonder if we just need to increase the default timeout here instead. Now it's 1 second and I think you're saying that leads to timeouts in your environment, let's raise the process_response timeout to maybe 3 or 5 seconds.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Add debug lines for flaky elixir tests [couchdb]

Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5702:
URL: https://github.com/apache/couchdb/pull/5702#discussion_r2430596629


##########
test/elixir/test/changes_async_test.exs:
##########
@@ -95,6 +95,7 @@ defmodule ChangesAsyncTest do
     create_doc_bar(db_name, "bar")
 
     changes = process_response(req_id.id, &parse_event/1)
+    Process.sleep(100)

Review Comment:
   Yeah it's curious how it works. Maybe adding some extra debug logs around parsing the result could reveal what's happening.
   
   I am not very familiar with Elixir, does it looks like `changes` is pulling data lazily when we call `length()` the first  time? Otherwise the way it looks is after `changes = ...` it shouldn't matter if we `sleep`, change is already fetched (either completely or partially).



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Add `retry_until` for flaky `changes_async_test.exs` tests [couchdb]

Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5702:
URL: https://github.com/apache/couchdb/pull/5702#discussion_r2436913859


##########
test/elixir/test/changes_async_test.exs:
##########
@@ -94,11 +94,13 @@ defmodule ChangesAsyncTest do
 
     create_doc_bar(db_name, "bar")
 
-    changes = process_response(req_id.id, &parse_event/1)
+    retry_until(fn ->

Review Comment:
   process_response doesn't seem to be idempotent so it's seems fishy that we retry at all here.
   
   The way it seems to work is it parses body lines from the socket or gets a timeout and returns. It could be that in one run we parse one line, then timeout, but on retry that 1st line won't be refetched, as far as the socket is concerned, it's already yielded the result. So on a retry we may get the second line only and still fail.
   
   It seems the original idea of sleeping a little would have worked better, however, I wonder if we just need to increase the default timeout here instead. Now it's 1 second and I think you're saying that leads to timeouts in your environment, let's raise the process_response timeout to maybe 3 or 5 seconds.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Add `retry_until` for flaky `changes_async_test.exs` tests [couchdb]

Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5702:
URL: https://github.com/apache/couchdb/pull/5702#discussion_r2436913859


##########
test/elixir/test/changes_async_test.exs:
##########
@@ -94,11 +94,13 @@ defmodule ChangesAsyncTest do
 
     create_doc_bar(db_name, "bar")
 
-    changes = process_response(req_id.id, &parse_event/1)
+    retry_until(fn ->

Review Comment:
   process_response doesn't seem to be idempotent so it seems fishy that we retry at all here.
   
   The way it seems to work is it parses body lines from the socket or gets a timeout and returns. It could be that in one run we parse one line, then timeout, but on retry that 1st line won't be refetched, as far as the socket is concerned it's already yielded the result. So on a retry we may get the second line only and still fail.
   
   It seems the original idea of sleeping a little would have worked better, however, I wonder if we just need to increase the default timeout here instead. Now it's 1 second and I think you're saying that leads to timeouts in your environment, let's raise the process_response timeout to maybe 3 or 5 seconds.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Add `retry_until` for flaky `changes_async_test.exs` tests [couchdb]

Posted by "jiahuili430 (via GitHub)" <gi...@apache.org>.
jiahuili430 commented on code in PR #5702:
URL: https://github.com/apache/couchdb/pull/5702#discussion_r2433667524


##########
test/elixir/test/changes_async_test.exs:
##########
@@ -95,6 +95,7 @@ defmodule ChangesAsyncTest do
     create_doc_bar(db_name, "bar")
 
     changes = process_response(req_id.id, &parse_event/1)
+    Process.sleep(100)

Review Comment:
   Found the `retry_until/3` function, this should work.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org