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

feat(#425): Create e2e test setup #610

Conversation

sugat009
Copy link
Member

@sugat009 sugat009 commented Jan 30, 2024

Description

Add a shell script that downloads the cht-core docker-helper file and sets cht-core up for e2e testing. The shell script also destroys cht-core containers using the same docker-helper after testing has been completed.

medic/cht-conf#[425]

Code review items

  • Readable: Concise, well named, follows the style guide, documented if necessary.
  • Documented: Configuration and user documentation on cht-docs
  • Tested: Unit and/or integration tests where appropriate
  • Backwards compatible: Works with existing data and configuration. Any breaking changes documented in the release notes.

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

@sugat009 sugat009 added the Type: Technical issue Improve something that users won't notice label Jan 30, 2024
@sugat009 sugat009 requested review from jkuester and m5r January 30, 2024 06:04
@sugat009 sugat009 self-assigned this Jan 30, 2024
1yuv and others added 2 commits January 30, 2024 20:42
@sugat009 sugat009 force-pushed the 425-improve-testing-on-cht-conf--setup-cht-core branch 2 times, most recently from 5f4bd5e to 61f4032 Compare February 1, 2024 08:49
Copy link
Contributor

@jkuester jkuester left a comment

Choose a reason for hiding this comment

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

Thanks for the incremental PRs here! I want to start with a broad question/suggestion about approach:

Is there a reason you think it might be better to use a shell script for the orchestration of the tests and to trigger the mocha run vs using mocha to orchestrate the setup and teardown of everything by running shell commands? (Shell script calling Mocha as you have done here vs Mocha calling shell commands....) Seems like either could work. 🤔

My main motivation for asking is that in cht-core, we settled on orchestrating everything (e.g. the integration tests) through mocha so that launching the tests via a mocha command will do all the setup/teardown of docker, etc. I guess doing the setup in mocha will result in more LoC overall (vs just a shell script), but it will be JS code1 which may be more accessible to cht-conf devs (folks like me that are not very savvy with shell scripts... 😅 ). IDK, @sugat009, let me know what you think here!

I was a bit skeptical at first glance about using the docker-helper to spin up the test instance, but the more I have thought about it, the more I like it! It is going to add some overhead complexity that might be challenging, but we will get a lot for free (such as avoiding port collisions and valid SSL certs). Seems like if we don't use the docker-helper we will just end up manually implementing the same thing.... In the future (does not have to be a part of this project), we should look into enhancing the docker-helper to be easier to use programmatically (so we don't have to echo "y"). Would be nice to just have a one-and-done command to run instead of the script having to be "interactive"....

Footnotes

  1. Admittedly it will be JS code that makes a bunch of shell calls via const { execSync, spawn } = require('child_process'); 🤷

@sugat009 sugat009 requested a review from lorerod February 5, 2024 12:27
@sugat009
Copy link
Member Author

sugat009 commented Feb 5, 2024

Is there a reason you think it might be better to use a shell script for the orchestration of the tests and to trigger the mocha run vs using mocha to orchestrate the setup and teardown of everything by running shell commands...

That's a very intriguing question @jkuester . I don't have a strong preference, but upon reflection, even though we utilize Mocha for setup and teardown orchestration, we essentially end up executing shell commands, albeit through JavaScript. In terms of choosing between the two, I'd say each approach has its own advantages and disadvantages. However, if I were to lean towards one, I'd opt for writing it in shell scripts. They are simpler to write, have lower resource overhead, and there's the rare scenario where a task is feasible in the shell but not as straightforward in a programming language like JS, TS, or Python.

My main motivation for asking is that in cht-core, we settled on orchestrating everything (e.g. the integration tests) through mocha so that launching the tests via a mocha command will do all the setup/teardown of docker, etc.

My apologies for not being aware of this. I must admit, I'm not a shell expert myself 😅. However, if the technical preference leans towards using JavaScript for these types of tasks, I have no objections and I'm willing to rewrite this in JS.

Would be nice to just have a one-and-done command to run instead of the script having to be "interactive"....

Yes, this would be a nice feature to have. I'm thinking of adding a flag to docker-helper such that it does not prompt any input and issues everything in default in the future.

@jkuester
Copy link
Contributor

jkuester commented Feb 5, 2024

@m5r @tatilepizs what are your thoughts on orchestrating the tests with a shell script vs Mocha? I agree with Sugat that a shell script will be more straightforward, but at the same time I put a lot of value in consistency and so it would be nice to match cht-core. @tatilepizs do you have any context for why we went with mocha for the cht-core e2e/integration tests?

@m5r
Copy link
Member

m5r commented Feb 6, 2024

I also like the simplicity of bash and would usually prefer it over javascript for tiny, well-defined scripts.
In this case I would lean towards javascript for two main reasons:

  • consistency, having one way of doing something across all Medic repos helps with maintainability
  • test runners like mocha provide nice utilities to catch errors and run things before/after to setup and tear down dependencies like spinning up and down docker containers for the tests

@tatilepizs
Copy link

I agree with @jkuester and @m5r about consistency, using different things could be complicated especially when we are learning about the project. I understand @sugat009's point about simplicity, but I really like the organization and structure of having just one way of doing things.

do you have any context for why we went with mocha for the cht-core e2e/integration tests?

No I don't, when I joined Medic that decision was already made 😅

However, I think that in one of the projects from ecosystem, they use something different than mocha to run tests. Am I right @lorerod ?

@lorerod
Copy link

lorerod commented Feb 7, 2024

Hi @m5r @jkuester @tatilepizs @sugat009, we experienced with Jest in the cht-interoperability repo, and it was mainly for unit and API tests. Our e2e test doesn't require web browser testing as the repo publishes an API, not a web interface. For API e2e testing, we use Jest with Supertest.
@medic/ecosystem's main arguments for choosing Jest in the cht-interoperability repository instead of the already-used Mocha were that Jest does not need other libraries to work and is focused on simplicity. We didn't want to overcomplicate a simple project with many extra dependencies. Our experience using jest was great.

I understand the benefits of having only one way of doing things. Cht-conf is more closely related to cht-core.
Does cht-conf have unit tests implemented? What was used for that? We could choose the same tool, which may also suit our needs for the e2e test.

@jkuester
Copy link
Contributor

jkuester commented Feb 7, 2024

That is helpful context @lorerod and thank you everyone for the input! cht-conf is currently using mocha for running the unit/integration tests. Based on the above feedback, I think it makes sense to go with a similar approach as in the cht-core integration/e2e tests where we use mocha as the entry-point/orchestrater of the test run. @sugat009 let me know if more clarity is needed here or if you run into something that makes this seem like a bad idea!

@sugat009
Copy link
Member Author

sugat009 commented Feb 8, 2024

Thanks, everyone for your feedback and input, it makes complete sense to go with mocha for orchestration for consistency. I'll revise my changes to match it with how we do it in cht-core using mocha.

@garethbowen
Copy link
Member

@sugat009 Just checking up with where we're at here?

@sugat009
Copy link
Member Author

@garethbowen This task has been on pause for a while now as I've been assigned other tasks such as medic/cht-android#308 and https://github.com/moh-kenya/config-echis-2.0/issues/1871. The progress before transitioning to pause is that I've re-written this in JS up until the point where the docker helper file is run and I was working on providing it input like in line 30 of test/e2e/test_runner.sh of the current PR state.

@m5r m5r force-pushed the 425-improve-testing-on-cht-conf--setup-cht-core branch from 9d89609 to 86a0e97 Compare July 1, 2024 11:42
@m5r m5r deleted the branch 425-improve-testing-on-cht-conf July 1, 2024 14:14
@m5r m5r closed this Jul 1, 2024
@m5r m5r deleted the 425-improve-testing-on-cht-conf--setup-cht-core branch July 1, 2024 14:21
@m5r
Copy link
Member

m5r commented Jul 1, 2024

Closing in favor of #620. Reason is Josh is out and there is no way to merge this to 425-improve-testing-on-cht-conf without him or an admin removing his request for changes.
The setup is done and orchestrated with mocha, I'm adding a test before going through a review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Technical issue Improve something that users won't notice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants