Skip to content

test: port coffee.js to Elixir test suite#1760

Merged
garrensmith merged 1 commit intoapache:masterfrom
dottorblaster:port-coffee-js-test-to-elixir
Nov 28, 2018
Merged

test: port coffee.js to Elixir test suite#1760
garrensmith merged 1 commit intoapache:masterfrom
dottorblaster:port-coffee-js-test-to-elixir

Conversation

@dottorblaster
Copy link
Copy Markdown
Member

Overview

I just ported coffee.js to the Elixir test suite.

Testing recommendations

Issue make elixir

Related Issues or Pull Requests

None

Checklist

  • Code is written and works correctly;
  • Changes are covered by tests;
  • Documentation reflects the changes;

Comment thread test/elixir/test/coffee_test.exs Outdated
use CouchTestCase

@moduletag :coffee
@headers ["X-Couch-Full-Commit": "false"]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not sure if we need this header any more. I ran this test with that header removed, and it also passed.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, I tried with and without the header and it was ~worthless, it seemed to get better but it didn't solve anything. We can drop it out eventually, I agree.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

With the new approach, the need for this header became absolute zero. I removed it indeed. Thanks!

Comment thread test/elixir/test/coffee_test.exs Outdated

# Giving time to the database to complete sync, otherwise values
# would be messed up and the test would be so much flaky
:timer.sleep(100)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Using a fixed sleep time is still very fragile (since CI machines can be much slower than your dev env), and I would recommend using a more robust synchronization technique. You could do something like:

  retry_until(fn ->
    %{"rows" => values} =
      Couch.get("/#{db_name}/_design/coffee/_view/myview",
        headers: @headers
      ).body
     assert 5 === hd(values)["value"]  
  end)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think we would have to discuss about an official way to check collection consistency right after an update 😬 this looks fine, I'll try. Thanks for pointing this out!

@dottorblaster dottorblaster force-pushed the port-coffee-js-test-to-elixir branch from b8f3229 to ec93044 Compare November 24, 2018 16:00
@dottorblaster
Copy link
Copy Markdown
Member Author

@jaydoane I think I came out with a cleverer approach in the end. Since we don't care about that specific execution order and we are testing the coffeescript query language here, now the test plays with documents first, then it receives the design document that builds the index. This way we work around the eventual consistency for the purpose of this test and we don't need any retry_until logic.

Thanks for pointing out that retry_until can be used anyway, I think it'll be useful for future ports! 👍 And thanks for the review man 😎

@wohali
Copy link
Copy Markdown
Member

wohali commented Nov 28, 2018

@dottorblaster Keeping in mind #1767, please re-run make elixir on this PR after rebasing on master and ensure that your new test conforms to the coding standard.

Once I have a +1 from you I'll merge this PR. Thanks!

@dottorblaster dottorblaster force-pushed the port-coffee-js-test-to-elixir branch from ec93044 to f4295bb Compare November 28, 2018 07:41
@dottorblaster dottorblaster force-pushed the port-coffee-js-test-to-elixir branch from f4295bb to 662b523 Compare November 28, 2018 07:43
@dottorblaster
Copy link
Copy Markdown
Member Author

@wohali done. Thanks!

@garrensmith garrensmith merged commit 78b0a7d into apache:master Nov 28, 2018
@dottorblaster dottorblaster deleted the port-coffee-js-test-to-elixir branch November 28, 2018 08:12
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.

4 participants