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

Process as test (VM -> Native test) #38

Merged
merged 14 commits into from
Aug 23, 2023
Merged

Process as test (VM -> Native test) #38

merged 14 commits into from
Aug 23, 2023

Conversation

shivaraj-bh
Copy link
Member

@shivaraj-bh shivaraj-bh commented Aug 8, 2023

Resolves #32

Run native test

just test

TODO

  • VM -> Native test
    • Postgres
    • Redis
    • Redis cluster

@srid
Copy link
Member

srid commented Aug 9, 2023

Could you add a Github Actions workflow for running the new tests? As well as a justfile entry for just test.

@shivaraj-bh
Copy link
Member Author

Another interesting point to consider here would be to only display the test logs, but filtering out all the other processes' logs might cause problems with visibility because if they fail we won't know why. Essentially we need a way to display logs of a given process if the process ended with a non-zero exit code.

@adrian-gierakowski
Copy link
Contributor

Another interesting point to consider here would be to only display the test logs

I’m currently doing this as follows

''
  ${pc-warapper} >/dev/null 2>&1 &
  pc_pid=$!

  ${pc-warapper} process logs test -f &
  test_logs_pid=$!

  wait $pc_pid
  kill $test_logs_pid
''

On first line you could redirect to tmp file and display last X lines if exit code is non-zero.

Note that displaying only the logs of the test process will get easier once this is addressed. Logs for all processes could then be collected using log_location at the top level of the config file.

@shivaraj-bh
Copy link
Member Author

On first line you could redirect to tmp file and display last X lines if exit code is non-zero.

Thanks! Will give it a shot.

@shivaraj-bh
Copy link
Member Author

CI works 🥳

@shivaraj-bh shivaraj-bh changed the title Process as test Process as test (VM -> Native test) Aug 10, 2023
@shivaraj-bh shivaraj-bh mentioned this pull request Aug 10, 2023
1 task
@srid
Copy link
Member

srid commented Aug 10, 2023

On macOS (Nix 2.17), I get:

❯ just test
nix flake check test/ --override-input services-flake . -L
warning: not writing modified lock file of flake 'git+file:///Users/srid/code/services-flake?dir=test':
• Updated input 'services-flake':
    'github:juspay/services-flake/f171932d130c3ce5184b69144012a225b2e28868' (2023-06-19)
  → 'git+file:///Users/srid/code/services-flake?ref=refs/heads/process-as-test&rev=9928f03781cba69263b83239c6c7bd26d6a1ef97' (2023-08-10)
error: builder for '/nix/store/c53s53w0xis4ak8jaw1df8cninh92d47-redis-test.drv' failed with exit code 1
error: build of '/nix/store/c53s53w0xis4ak8jaw1df8cninh92d47-redis-test.drv', '/nix/store/d9ssfys8nawjym0cgm26g6m5czamhzpa-redis-cluster-test.drv' failed
error: Recipe `test` failed on line 14 with exit code 1

This is odd,

❯ nix log /nix/store/c53s53w0xis4ak8jaw1df8cninh92d47-redis-test.drv
warning: The interpretation of store paths arguments ending in `.drv` recently changed. If this command is now failing try again with '/nix/store/c53s53w0xis4ak8jaw1df8cninh92d47-redis-test.drv^*'

Tests run on NixOS though.

.github/workflows/ci.yaml Outdated Show resolved Hide resolved
nix/redis-cluster_test.nix Outdated Show resolved Hide resolved
@shivaraj-bh
Copy link
Member Author

shivaraj-bh commented Aug 11, 2023

On macOS (Nix 2.17), I get:

❯ just test
nix flake check test/ --override-input services-flake . -L
warning: not writing modified lock file of flake 'git+file:///Users/srid/code/services-flake?dir=test':
• Updated input 'services-flake':
    'github:juspay/services-flake/f171932d130c3ce5184b69144012a225b2e28868' (2023-06-19)
  → 'git+file:///Users/srid/code/services-flake?ref=refs/heads/process-as-test&rev=9928f03781cba69263b83239c6c7bd26d6a1ef97' (2023-08-10)
error: builder for '/nix/store/c53s53w0xis4ak8jaw1df8cninh92d47-redis-test.drv' failed with exit code 1
error: build of '/nix/store/c53s53w0xis4ak8jaw1df8cninh92d47-redis-test.drv', '/nix/store/d9ssfys8nawjym0cgm26g6m5czamhzpa-redis-cluster-test.drv' failed
error: Recipe `test` failed on line 14 with exit code 1

This is odd,

❯ nix log /nix/store/c53s53w0xis4ak8jaw1df8cninh92d47-redis-test.drv
warning: The interpretation of store paths arguments ending in `.drv` recently changed. If this command is now failing try again with '/nix/store/c53s53w0xis4ak8jaw1df8cninh92d47-redis-test.drv^*'

Tests run on NixOS though.

This also happens on my macOS with Nix 2.13.3. Additionally I also tried running the example flake with just ex after updating nixpkgs in example/flake.lock to use the latest PC and the example app doesn't run at all.

I tried on different macOS (with Nix 2.15.1) where PC was never ran before and it seems to work fine there. This is weird, have to debug this.

By MacOS I mean aarch64-darwin in both the devices mentioned above.

@shivaraj-bh
Copy link
Member Author

shivaraj-bh commented Aug 16, 2023

@srid just test is working on mac, the error wasn't conclusive but it looks like having each PC package run on different swagger endpoint port makes it work.

A nicer implementation will be to disable swagger endpoint so that we won't have this conflict.

test/flake.nix Outdated Show resolved Hide resolved
@srid
Copy link
Member

srid commented Aug 16, 2023

A nicer implementation will be to disable swagger endpoint so that we won't have this conflict.

Yea, I was thinking of an upstream contribution to that end. Or we can set port to 0 by default (in process-compose-flake), forcing the user to set it explicitly if they want it.

just test runs fine for me now.

@srid
Copy link
Member

srid commented Aug 16, 2023

Display test logs if non-zero exit code

Can we do this separately in a different PR? I feel like we won't even need it, but let's see if a need actually arises over time.

Copy link
Member

@srid srid left a comment

Choose a reason for hiding this comment

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

LGTM once process-compose-flake is updated.

@shivaraj-bh
Copy link
Member Author

@srid Looks like the CI is running forever when I set disabled = true; for test process

@shivaraj-bh
Copy link
Member Author

Looks like we can only override keys that are not already set in the base yaml configuration. We can either upstream the change in process-compose or use two overlays (one for package that disables test and another for testPackage that enables it and adds exit_on_end).

@srid
Copy link
Member

srid commented Aug 22, 2023

Oh, I thought we are automatically disabling the test process. I think we should upstream a change to process-compose-flake such that outputs.package too uses an override config that disables the test process if there is one. The user doesn't have to set disabled = true manually on their test process.

After all, "test" has a special name in process-compose-flake, and it should behave uniformly across all outputs it produces.

Copy link
Member

@srid srid left a comment

Choose a reason for hiding this comment

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

Let's get this merged, after switching upstream flake url.

@shivaraj-bh
Copy link
Member Author

Done!

@srid
Copy link
Member

srid commented Aug 23, 2023

Great, you can (squash) merge it yourself.

@shivaraj-bh shivaraj-bh merged commit 72a3eaa into main Aug 23, 2023
@shivaraj-bh shivaraj-bh deleted the process-as-test branch August 23, 2023 17:17
@srid srid mentioned this pull request Mar 12, 2024
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.

Replace vm tests with native tests
3 participants