james-stocks commented 5 years ago

(NOTE: this issue template and review process for this issue follow the updated Chef OSS practices Comments with a 👍 reaction have been addressed (either in changes to this design or in subsequent comments) - readers wishing to catch up can ignore comments with a 👍 reaction and review the current issue text + any remaining comments not marked 👍 )

core-plans testing standard

Chef wants to ensure the highest level of quality and best developer experience for projects that choose to build their software with Habitat. Part of that goal is to set a standard for quality assurance in this repository.

Motivation

As a core-plans maintainer,
I want to have basic testing for all core plans,
so that core-plans is easier to maintain, and build issues do not manifest downstream.

Specification

There are two aspects to this design:

Defining the scope of testing that is appropriate for this project, and
Defining how tests that are in scope will be implemented.

Scope of Testing

It's essential to define and agree on the scope of testing. Testing must cover all aspects of quality that this repository is responsible for; and testing must also not exceed what the code in this repository does - if a failing test would not be fixed via a change to this repository, the test does not belong in this repository.

In-Scope for Testing

Task	Test
Each Core Plan should build	Exits 0, outputs a .hart file
Each Core Package should install.	Pkg install
Core Plans with executables should produce valid executables.	Executable exists and executes in (at least) the most basic way e.g. `--help`
Core plans list their required runtime dependencies	Run a dependency checker tool on libraries and executables built by the plan (e.g. ldd or depends.exe)
The Core Plan builds the version stated in the plan file	Perform the software's version check and compare to the plan file
Plans with services should run with minimum viable configuration.	Working run, init, and reconfig hooks
Plans with services can upgrade a running instance of the previous release of that service.	Install the current stable version of the package from builder and run it with an upgrade strategy of `at-once`.

Out-of-Scope for Testing

Assumptions	Exclusions
Habitat is healthy	No tests exercising only Habitat
Upstream software takes care of its own quality	No tests beyond the do_check() hook passing in CI
Core Plans always aim to have minimal complexity	No complexity to the test scenarios beyond testing what the core plan is responsible for
Integration between Core Plan services is a higher level of testing that will come later	No integration testing.

Implementation of Automated Tests in core-plans

CI configuration

core-Plans currently has CI configuration to detect what plans have been modified and need tested (see here). If a core plan has a tests directory, the contents should be run during CI for a Pull Request.

Proposal for CI

CI should execute tests/test.sh and tests/test.ps1 if either exists inside the directory of a plan that has been modified.
- Some packages provide both a Linux and a Windows plan; so CI should have a build step for each.
tests/test.sh or tests/test.ps1 will not require any pre-installed software other than what is available on the CI build agent.
For the time being, the build should not fail if tests/test.sh or tests/test.ps1 does not exist.
- Longer term, all plans should have some testing and new plans should not be accepted without them.
The CI build will be marked failed if tests/test.sh or tests/test.ps1 exits with a non-zero exit code.
The tests should be separate from the build - CI will build the package before calling tests/test.sh or tests/test.ps1, with the fully qualified package ident of the built package to test as a parameter.

Test implementation

The standard testing programs used for core-plans are BATS (for Linux) and Pester (for Windows).
Tests should not execute inside the studio instance than the package was built in; to avoid cases where the software works only due to build dependencies being present.
- Tests should run independently from the build and install of the package. The test script (tests/test.sh / tests/test.ps1 ) should take the fully qualified package ident for a currently installed package as a parameter.
- Tests for built binaries should use hab pkg exec to execute the binary. hab pkg binlink should not be used. See the [Appendix] for some examples to demonstrate why.

`tests` folder structure

Structure below is proposed as common format for the core plan testing directory with the following features:

./tests/test.sh script runs the build, install, and tests and must work on a CI build agent.
- There is an existing convention to set the environment variable SKIPBUILD to have the test script only run tests - new plans should implement this.
./tests/bats and ./tests/inspec are the directories for respective tests. If a community member insists on using bats, then he can and place them beneath the appropriate directory. If we decide to do all inspec then we would place ours beneath the appropriate directory.

  └── tests
   ├── test.sh
   ├── test.bats
   ├── test.ps1
   └── test.pester.ps1

Downstream Impact

There should be no negative downstream impacts. Builder will continue to build new unstable packages when commits are merged in this repository. It is intended that tests implemented according to this design can be run against artifacts on builder (or other downstream environments); but how that will actually happen is future work.

Appendix

Examples of why `hab pkg exec` should be used

Missing dependencies can be masked

[16][default:/src:0]# cat bin/mysed
#!/bin/sh

sed "$@"
[17][default:/src:0]# cat plan.sh
pkg_name="mysed"
pkg_version="0.0.1"
pkg_bin_dirs=(bin)
do_build() { :; }
do_install() {
    cp $PLAN_CONTEXT/bin/mysed $pkg_prefix/bin/mysed
}

[14][default:/src:0]# hab pkg exec smacfarlane/mysed mysed --help
/hab/pkgs/smacfarlane/mysed/0.0.1/20190410152301/bin/mysed: line 3: sed: command not found

[15][default:/src:127]# /hab/pkgs/smacfarlane/mysed/0.0.1/20190410152301/bin/mysed --help
Usage: sed [OPTION]... {script-only-if-no-other-script} [input-file]...

The program may execute with a different PATH

[10][default:/src:1]# /hab/pkgs/core/coreutils/8.30/20190115012313/bin/env
...
PATH=/hab/pkgs/core/hab-plan-build/0.78.0/20190313122208/bin:/hab/pkgs/core/diffutils/3.6/20190115013221/bin:/hab/pkgs/core/less/530/20190115013008/bin:/hab/pkgs/core/make/4.2.1/20190115013626/bin:/hab/pkgs/core/mg/20180408/20190115015655/bin:/hab/pkgs/core/util-linux/2.32/20190115013746/bin:/hab/pkgs/core/vim/8.1.0577/20190115015449/bin:/hab/pkgs/core/ncurses/6.1/20190115012027/bin:/hab/pkgs/core/acl/2.2.53/20190115012136/bin:/hab/pkgs/core/attr/2.4.48/20190115012129/bin:/hab/pkgs/core/bash/4.4.19/20190115012619/bin:/hab/pkgs/core/binutils/2.31.1/20190115003743/bin:/hab/pkgs/core/bzip2/1.0.6/20190115011950/bin:/hab/pkgs/core/coreutils/8.30/20190115012313/bin:/hab/pkgs/core/file/5.34/20190115003731/bin:/hab/pkgs/core/findutils/4.6.0/20190115013303/bin:/hab/pkgs/core/gawk/4.2.1/20190115012752/bin:/hab/pkgs/core/glibc/2.27/20190115002733/bin:/hab/pkgs/core/grep/3.1/20190115012541/bin:/hab/pkgs/core/gzip/1.9/20190115013612/bin:/hab/pkgs/core/hab/0.78.0/20190313115951/bin:/hab/pkgs/core/libcap/2.25/20190115012150/bin:/hab/pkgs/core/openssl-fips/2.0.16/20190115014207/bin:/hab/pkgs/core/openssl/1.0.2r/20190305210149/bin:/hab/pkgs/core/pcre/8.42/20190115012526/bin:/hab/pkgs/core/rq/0.10.4/20190115014520/bin:/hab/pkgs/core/sed/4.5/20190115012152/bin:/hab/pkgs/core/tar/1.30/20190115012709/bin:/hab/pkgs/core/unzip/6.0/20190115014516/bin:/hab/pkgs/core/wget/1.19.5/20190305211748/bin:/hab/pkgs/core/xz/5.2.4/20190115013348/bin:/bin

[11][default:/src:0]# hab pkg exec core/coreutils env
...
PATH=/hab/pkgs/core/coreutils/8.30/20190115012313/bin:/hab/pkgs/core/glibc/2.27/20190115002733/bin:/hab/pkgs/core/acl/2.2.53/20190115012136/bin:/hab/pkgs/core/attr/2.4.48/20190115012129/bin:/hab/pkgs/core/libcap/2.25/20190115012150/bin

The program may not have required env vars

[14][default:/src:0]#  hab pkg exec core/ansible ansible --version
ansible 2.7.6
  config file = /hab/pkgs/core/ansible/2.7.6/20190305222954/etc/ansible.cfg
  configured module search path = [u'/root/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /hab/pkgs/core/ansible/2.7.6/20190305222954/lib/python2.7/site-packages/ansible-2.7.6-py2.7.egg/ansible
  executable location = /hab/pkgs/core/ansible/2.7.6/20190305222954/bin/ansible
  python version = 2.7.15 (default, Mar  5 2019, 21:30:01) [GCC 8.2.0]

[16][default:/src:1]# hab pkg binlink core/ansible ansible
» Binlinking ansible from core/ansible into /bin
★ Binlinked ansible from core/ansible/2.7.6/20190305222954 to /bin/ansible
[17][default:/src:1]# ansible --version
Traceback (most recent call last):
  File "/bin/ansible", line 4, in <module>
    __import__('pkg_resources').run_script('ansible==2.7.6', 'ansible')
  File "/hab/pkgs/core/python2/2.7.15/20190305212819/lib/python2.7/site-packages/pkg_resources/__init__.py", line 3088, in <module>
    @_call_aside
  File "/hab/pkgs/core/python2/2.7.15/20190305212819/lib/python2.7/site-packages/pkg_resources/__init__.py", line 3072, in _call_aside
    f(*args, **kwargs)
  File "/hab/pkgs/core/python2/2.7.15/20190305212819/lib/python2.7/site-packages/pkg_resources/__init__.py", line 3101, in _initialize_master_working_set
    working_set = WorkingSet._build_master()
  File "/hab/pkgs/core/python2/2.7.15/20190305212819/lib/python2.7/site-packages/pkg_resources/__init__.py", line 574, in _build_master
    ws.require(__requires__)
  File "/hab/pkgs/core/python2/2.7.15/20190305212819/lib/python2.7/site-packages/pkg_resources/__init__.py", line 892, in require
    needed = self.resolve(parse_requirements(requirements))
  File "/hab/pkgs/core/python2/2.7.15/20190305212819/lib/python2.7/site-packages/pkg_resources/__init__.py", line 778, in resolve
    raise DistributionNotFound(req, requirers)
pkg_resources.DistributionNotFound: The 'ansible==2.7.6' distribution was not found and is required by the application

echohack commented 5 years ago

Hi @james-stocks

This repo has not yet adopted the new open source practices, so you have several options here:

Submit this as an RFC to the existing RFC repo process: https://github.com/habitat-sh/core-plans-rfcs
Wait until the maintainers have converted all of the existing RFCs to be in line with the new OSS design process and submit it then.

smacfarlane commented 5 years ago

Proposal for CI

Recommendation: We should use CI in place of Buildkite, unless we specifically need to refer to the Buildkite service. This separates the idea of "We should run automated tests" from "we have chosen Buildkite to run the tests". Also, while I don't expect it to change anytime soon, we'd have to rewrite this if we were to change our CI service 😄
* `tests/test.sh` and `tests/test.ps1` will build the package before running the tests.

I'm a pretty hard 👎 on this. tests/test.sh should take an artifact as input and not concern itself with the build at all. This gives us the flexibility to use the same set of tests against artifacts from builder, allowing us to test the results of a build group, though as you note in the "downstream impacts" section this is a separate topic on if/how that gets wired up.

The scripts also shouldn't have the concept of maybe build. I'd want to verify on every CI run that the environment was set up correctly such that it didn't accidentally build the package because the interface in deciding how to build changed, or something in CI causes an inadvertent change in behavior.

* ./tests/test.sh script runs the build, install, and tests and must work on a Buildkite build agent.

  * There is an existing convention to set the environment variable `SKIPBUILD` to have the test script only run tests - new plans should implement this.

Same comments as above.

echohack commented 5 years ago

@james-stocks Also I want to assure you that I'm not giving you the run around on this topic. I know you and @gavindidrichsen are working hard on providing tests for core-plans.

It's best that we solve these problems individually and discuss these things in long form. Just to be clear, there are several things happening right now:

Discussion around converting existing RFCs to the new OSS design process
Discussion around core-plans testing standards (this issue)
Discussion around wrapper/service plans inside of core-plans

These discussions are somewhat dependent on each other, but I think it's safe to say that this is the order that we should solve them in, as it will help make each subsequent problem a little easier to solve.

Again, thank you for your patience as the community and maintainers work to solve these problems!

smacfarlane commented 5 years ago

@echohack I don't think this needs be closed out or moved to an RFC while the migration from RFC to design proposal is in progress.

I agree that this should remain open until we've done the appropriate work for that migration though.

I encourage people to continue to comment on this issue in the mean time.

james-stocks commented 5 years ago

We should use CI in place of Buildkite

Updated the issue.

tests/test.sh should take an artifact as input and not concern itself with the build at all. This gives us the flexibility to use the same set of tests against artifacts from builder

I have a little uncertainty here because one of the main tests for plans is to query the software's version and compare it to what should be built e.g.

source "${BATS_TEST_DIRNAME}/../plan.sh"

@test "Version matches" {
  result="$(rg --version | head -1 | awk '{print $2}')"
  [ "$result" = "${pkg_version}" ]
}

If we design the tests to be re-used then we need some mechanism to get the expected version of the software that works both for building the plan from a git clone of this repo (i.e. the CI here) and when testing as a builder artifact. Maybe there is some directory and file under /hab/pkgs that will present in any case, that the test can read?

Aside from that, it sounds like you are suggesting that:

The tests assume a package is built and installed.
- For plans that provide services, the tests will still control starting and/or stopping the service in the supervisor.
The README will guide contributors to build and install before running tests.
The CI will build and install the plan before invoking the test script.

Is that right, or were you thinking of something different?

This sounds good to me; but it differs from how plans for tests are written today and we'd need to handle existing plan tests failing in this new CI setup.

smacfarlane commented 5 years ago

In my opinion, we shouldn't be sourcing the plan in the test. That blurs the line between testing the resulting artifact and testing that plan-build did what it said it would.

I also don't think testing the version provides value for the same reason, with the possible exception of builds that use version control that is not easily controlled by plan-build. Go based packages come to mind.

In these cases it may be fine to expect certain variables to be defined in the environment, such as ones that can be sourced from last_build.env. That still feels a little off to me though. We don't provide a VERSION file in the metadata of a package, only IDENT, so there's no easy win there without writing libraries 😄

My personal belief is that CI should drive the flow, and allow each discrete piece (in this case tests) to focus on their role in the process. If a user wants to run the tests locally, they should be able to run the CI script directly if the platform allows it.

I don't want dive into solutioneering in this, but to illustrate:

build <input>   # plan-build always takes a plan directory as input. 
source results/last_build.env # plan-build always provides us with this as an output, which we can use to get the $pkg_ident of the thing we just built.
<input>/test.sh $pkg_ident  # Similar to plan-build, the input contract is always a single item, in this case a fully qualified ident.

No matter how this discussion goes, I think we'll have to grandfather existing tests until they have a PR opened against them and adjust according on a case-by-case basis to bring them in line with our final decision.

bdangit commented 5 years ago

Why don’t we have the last_build.env have the pkg_version ?

echohack commented 5 years ago

@bdangit

That seems like an easy thing to add... :)

https://github.com/habitat-sh/habitat/blob/0359ba8665f724d1be1802093f50d7ecc2345da0/components/plan-build/bin/hab-plan-build.sh#L2275

Edit: Wait, it already has pkg_version, soooo....

It's seems obvious to me that the version check could easily reference last_build.env already!

james-stocks commented 5 years ago

My uncertainty about using last_build.env relates to this comment from @smacfarlane :

This gives us the flexibility to use the same set of tests against artifacts from builder, allowing us to test the results of a build group

If the tests are being run against an artifact from builder, there will not be a last_build.env file.

It looks like we can determine it from /hab/pkgs/core/[PACKAGE]/[VERSION]/[STAMP]/IDENT ?

smacfarlane commented 5 years ago

If the tests are being run against an artifact from builder, there will not be a last_build.env file.

Yup, this is in part why I think the test should take a fully qualified package ident as a parameter and not source anything. CI will know what it just built and pass that identifier to the tests. Likewise builder will know the idents of the things that need to be tested (whatever shape that work ends up taking).

smacfarlane commented 5 years ago

Test implementation

I'd like to propose an addition to this section,

Tests should not use hab pkg binlink.

When executing commands that have been "binlinked", you lose environmental correctness that hab pkg exec and the lifecycle hooks give you. I've put together a couple examples to demonstrate:

Simple sed wrapper that demonstrates how "binlinking" can mask missing dependencies: https://gist.github.com/smacfarlane/d92fef5b7f4030c0027c285e2310e2e5#file-sed-wrapper

An example of compiled software that has a different PATH depending on how it was executed, leading to potentially executing a version of software you didn't explicitly depend on: https://gist.github.com/smacfarlane/d92fef5b7f4030c0027c285e2310e2e5#file-coreutils-example

An example of interpreted software that relies on environmental variables being present in order to function:
https://gist.github.com/smacfarlane/d92fef5b7f4030c0027c285e2310e2e5#file-env-vars-example

james-stocks commented 5 years ago

Tests should not use hab pkg binlink

I agree with this but would like to word it as:

Tests should use hab pkg exec and should not use hab pkg binlink

...because the examples you gave all demonstrate why to use hab pkg exec but do not all use binlink to demonstrate the pitfall.

I hope it is OK to copy your example gists directly into this design?

james-stocks commented 5 years ago

"Downstream Impact" should possibly be re-worded because it says these tests are not used on builder; but part of the discussion above is that we should write tests that could be re-used on builder

james-stocks commented 5 years ago

tests/test.sh and tests/test.ps1 will build the package before running the tests.

This statement should change, I agree with @smacfarlane on

the test should take a fully qualified package ident as a parameter and not source anything. CI will know what it just built and pass that identifier to the tests. Likewise builder will know the idents of the things that need to be tested (whatever shape that work ends up taking).

I would suggest standardising on an env var that should be set for the tests e.g. PACKAGE_IDENT. I suggest an env var rather than passing the ident as a parameter on CLI because at least BATS does not support arguments - it will need to find the package ident either in the environment or in a file.

smacfarlane commented 5 years ago

I suggest an env var rather than passing the ident as a parameter on CLI because at least BATS does not support arguments - it will need to find the package ident either in the environment or in a file.

I do think it should be a parameter, as that makes the input to the tests explicit when called. You would then have to set and export the expected env var in bats which does feel kinda silly.

I don't have a strong preference either way, as long as we're consistent. The above is why I recommended a parameter though.

james-stocks commented 5 years ago

Let's go with parameter. It looks like Pester accepts parameters at the command line so BATS would be the odd-one-out in this case.

james-stocks commented 5 years ago

I'm unsure about this bit:

For the time being, the build should not fail if tests/test.sh or tests/test.ps1 does not exist.

This might cause confusion because a PR reviewer is going to have to dig into the CI build to see if tests executed and passed. It's unhelpful for a human to need to go and inspect a CI build that is green. The alternative is to fail the build if tests are missing - this will be an inconvenience to someone making a small contribution and having the burden of setting up tests put on them.

This problem will only be temporary as we aim to add tests to all plans.

Edit: by coincidence I just noticed @tduffield mention that BuildKite now supports a soft_fail and that might be an option in this case.

predominant commented 5 years ago

Is the first Comment updated with all comments here? I have not have time to review this as yet.

james-stocks commented 5 years ago

I believe I've worked all the comments into the original issue except for my own question in the last comment.

I guess I should :+1: any comment that's been resolved.

On Tue, 30 Apr 2019, 5:26 pm Graham Weldon, notifications@github.com wrote:

Is the first Comment updated with all comments here? I have not have time to review this as yet.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/habitat-sh/core-plans/issues/2473#issuecomment-488020414, or mute the thread https://github.com/notifications/unsubscribe-auth/AC4TBWMCNOZDQWEAEQ2JMYTPTBXKJANCNFSM4HGT3OHQ .

james-stocks commented 5 years ago

The review period for this design has ended. As per the OSS design process, I've raised a PR to commit this to the project: https://github.com/habitat-sh/core-plans/pull/2539

habitat-sh / core-plans

Design proposal - core-plans testing standard #2473

core-plans testing standard

Motivation

Specification

Scope of Testing

In-Scope for Testing

Out-of-Scope for Testing

Implementation of Automated Tests in core-plans

CI configuration

Proposal for CI

Test implementation

`tests` folder structure

Downstream Impact

Appendix

Examples of why `hab pkg exec` should be used

Proposal for CI

habitat-sh / core-plans

Design proposal - core-plans testing standard #2473

core-plans testing standard

Motivation

Specification

Scope of Testing

In-Scope for Testing

Out-of-Scope for Testing

Implementation of Automated Tests in core-plans

CI configuration

Proposal for CI

Test implementation

tests folder structure

Downstream Impact

Appendix

Examples of why hab pkg exec should be used

Proposal for CI

`tests` folder structure

Examples of why `hab pkg exec` should be used