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

Nixify io and memory tests #1538

Merged

Conversation

monacoremo
Copy link
Member

@monacoremo monacoremo commented May 26, 2020

As discussed in #1535, this integrates the io and memory tests with the Nix setup.

A few notes:

  • The io and memory scripts are now independent from stack and take the postgrest executable from the PATH (also reflected in the stack CI jobs)
  • This PR touches quite a few files because the db URI needed to be parameterized (the with_tmp_db will never be in the same location) - otherwise I tried to keep the changes minimal
  • The tests still feels a bit hacky/brittle - the nmap dependency could probably be removed (its process not being properly terminated currently), as could the dependency on a hardcoded port. Shellcheck throws many warnings on the two scripts, but I left as much as possible as it was for this PR :-)
  • The Nix test scripts now depend on the final executables (both a static and profiled build). This is good on the one hand, as that makes the tests truly end-to-end. On the other hand, it would make the nix-shell startup times very large if we included the tests by default. So I implemented flags to enable the more 'expensive' modules and also reflected that in the README

@monacoremo monacoremo force-pushed the monacoremo-nix-io-memory-tests branch from 76efa2d to e29ef9b Compare May 26, 2020 21:25
@monacoremo monacoremo marked this pull request as ready for review May 26, 2020 23:00
nix/README.md Outdated
Comment on lines 77 to 78
Some additional modules like `tests`, `docker` and `release` have large
dependencies that would need to be built before the shell becomes available,
Copy link
Member

@steve-chavez steve-chavez May 27, 2020

Choose a reason for hiding this comment

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

Hmm.. couldn't we have the end-to-end tests(running io-tests.sh and memory-tests.sh) in another nix file(like io-tests.nix) and avoid building the static and profiled builds on nix-shell entrance?

I was really liking the DX of going inside nix-shell fast and being able to do postgrest-test-<tab>.

Copy link
Member

Choose a reason for hiding this comment

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

Then we could do nix-shell --arg io-tests true in case we need to run the io tests. I think this would be better because those tests aren't run that frequently.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, that would be much nicer! Didn't think about that.

With the new commit the spec tests are available in the nix-shell by default and the other tests can be enabled with flags!

Copy link
Member

Choose a reason for hiding this comment

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

@monacoremo Thanks! Much better now.

Just something I noticed. Looks like the nix memory tests consume less memory than the stack-test-memory.

nix-tests:

Running memory usage tests..
ok 1 - POST /rpc/leak?columns=blob: with a json key of 1M the memory usage(9,075,584 bytes) is less than 13M
ok 2 - POST /leak?columns=blob: with a json key of 1M the memory usage(9,193,768 bytes) is less than 13M

stack-memory-test:

ok 1 - POST /rpc/leak?columns=blob: with a json key of 1M the memory usage(12,122,480 bytes) is less than 13M
ok 2 - POST /leak?columns=blob: with a json key of 1M the memory usage(12,257,056 bytes) is less than 13M

Not sure why. But it's good nonetheless, the stack memory tests used to break from time to time.

Copy link
Member Author

Choose a reason for hiding this comment

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

@steve-chavez Interesting... I don't have much experience with profiling haskell, might it be that some libraries are not included in the nix profiled build? Or different optimization levels?

@@ -231,8 +234,10 @@ readSecretFromFile ascii.b64 'Base64 (ASCII)'
readSecretFromFile utf8.b64 'Base64 (UTF-8)'
readSecretFromFile binary.b64 'Base64 (binary)'

readDbUriFromFile uri.noeol "(no EOL)"
readDbUriFromFile uri.txt "(EOL)"
eol=$'\x0a'
Copy link
Member

Choose a reason for hiding this comment

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

Nice. Thanks for taking the time to refactor these tests! Comments are really good too.

pgrStopAll(){ pkill -f "$(stack path --profile --local-install-root)/bin/postgrest"; }

pgrStart(){ stack exec --profile -- postgrest test/memory-tests/config +RTS -p -h >/dev/null & pgrPID="$!"; }
pgrStart(){ postgrest test/memory-tests/config +RTS -p -h > /dev/null & pgrPID="$!"; }
Copy link
Member

Choose a reason for hiding this comment

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

Using the binary is much better. I remember I had some issues with stack there.

@steve-chavez
Copy link
Member

Looks good to merge now!

The tests still feels a bit hacky/brittle - the nmap dependency could probably be removed (its process not being properly terminated currently), as could the dependency on a hardcoded port. Shellcheck throws many warnings on the two scripts, but I left as much as possible as it was for this PR :-)

The tests are definitely hacky, never took the time to run shellcheck on them. It'd be great if you could further improve them on a later PR.

@steve-chavez steve-chavez merged commit 69b09e3 into PostgREST:master May 28, 2020
monacoremo added a commit to monacoremo/postgrest that referenced this pull request Jul 17, 2021
* Include tests for io and memory in the Nix environment.
* include spec tests in nix-shell by default
* install the io and memory tests in CI
@monacoremo monacoremo deleted the monacoremo-nix-io-memory-tests branch July 27, 2021 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants