Skip to content

Conversation

@stanislaw
Copy link
Member

@stanislaw stanislaw commented Oct 30, 2019

Background

Recently there have been some open discussions and open pull requests that require some attention of the maintainers and they are more or less advanced which makes it not easy for them to be merged without a deep dive into their specific details.

All of these discussions and a number of related pending changesets made me think that we need to have a higher level of confidence in the new patches that are coming into the main development branch.

Examples:

Proposed solution

This can be seen as a cross-pollination between doorstop and another project that I work on: mutation testing system for C/C++ based on LLVM: https://github.com/mull-project/mull.

We have started using LLVM LIT for integration tests because some time ago we encountered an issue of losing control over the quality of the master branch inspite of our efforts to maintain a very decent unit tests coverage and some integration tests across a number of LLVM versions and macOS/Linux platforms.

The idea of LLVM Integrated Tester is better explained here but I can also recommend a very nice written by my friend and colleague @AlexDenisov: https://lowlevelbits.org/system-under-test-llvm/.

The idea behind this changeset is to show you how I see LIT could be applied in the context of doorstop:

  • Every test case is made as small as possible: there is some input like an empty folder or a folder with some prepared doorstop files (Input), there is a doorstop <command action and there is a folder Expected which contains the expected outcome
  • For every test case, a Sandbox folder is created from scratch where the test case input is deployed to and the doorstop command is run.
  • The test case verifies two things:
    • That the contents of the Sandbox and Expected folders are identical.
    • The LIT test case also greps for the output produced by doorstop executable and there we capture the most important lines that we want to be there.

If this approach is accepted, I am happy to provide the coverage for all existing functionality. I have acquired some experience of working with LIT so I am happy to put the basic knowledge I now have into the test cases from where it will be obvious how to use LIT at most of its power.

Additionally, I am happy to pay a special attention to creating a special set of test cases that will focus on the Core Doorstop functionality that, as we will have to agree, has to be there 100% so that any diverging changes will never affect how the Core Doorstop works. I think this core integration tests will especially help the main maintainer @jacebrowning to have a good confidence that the diverging use cases do not result in an accidental corruption of the main feature set. At the same time, things like requirements with letters instead of numbers proposed by @sebhub can safely co-exist under additional commands/options.

Technical details

-lit itself is installed with pip so I need your help @jacebrowning to know where/how to put it to be auto-installed.

  • FileCheck is a binary precompiled by me on macOS from existing LLVM 8 source code. The library seems to be portable across machines so I am almost sure having the same binary precompiled for Linux and having them both in the source code will be enough for them to run on any machine.

  • The compare_dirs work ok-ish now but can be improved later or replaced with a python version.

@jacebrowning
Copy link
Member

All test dependencies go in the [tool.poetry.dev-dependencies] section of pyproject.toml to be installed with $ make install.

@stanislaw
Copy link
Member Author

stanislaw commented Oct 30, 2019

@jacebrowning,

It looks like the presence of the tests/integration folder makes some of the tests fail on CI. It is also the case on my local machine: if I just delete the tests/integration folder, the tests pass.

I guess this has to do with the presence of .doorstop.yml in the changeset. Is there a way I can exclude the tests/integration folder from being considered by the existing test suite?

The next step for me is to create a FileCheck binary for Linux/Xenial and see if it works portably inside my Docker images and on the Travis CI machine.

@stanislaw
Copy link
Member Author

It looks like the presence of the tests/integration folder makes some of the tests fail on CI. It is also the case on my local machine: if I just delete the tests/integration folder, the tests pass.

I guess this has to do with the presence of .doorstop.yml in the changeset. Is there a way I can exclude the tests/integration folder from being considered by the existing test suite?

If there is no easy way to do it we could introduce a feature to make doorstop ignore the folders that have something like .doorstop-ignore in them. This would reduce the interference between different test groups.

What do you think? @jacebrowning @sebhub

@stanislaw
Copy link
Member Author

@jacebrowning @sebhub

I need to know if you guys think having these lit-tests is a good idea. If you give me a go, I can implement the .doorstop-ignore feature right here in the branch and even implement a lit-test for it. Then I would make everything green and mergeable.

Please share your thoughts.

@sebhub
Copy link
Member

sebhub commented Oct 31, 2019

I am not sure about the .doorstop-ignore. How is it different from the existing .doorstop.skip?

Tomorrow is a public holiday in Bavaria and I am on holidays for the next three days.

@stanislaw
Copy link
Member Author

stanislaw commented Oct 31, 2019

I am not sure about the .doorstop-ignore. How is it different from the existing .doorstop.skip?

Thanks for the answer. I simply didn't know about .doorstop.skip. However it does not work the way I would like it to work: putting .doorstop.skip into tests/integration does not prevent the existing test suite from looking at the files inside tests/integration and colliding with them with:

...
doorstop.core.tree: INFO: root of the tree: TST
doorstop.cli.utilities: ERROR: multiple root documents
doorstop.cli.main: DEBUG: command failed

The desired behavior would be that Doorstop does not even look further inside the tests/integration once it sees .doorstop.skip in it.

@sebhub
Copy link
Member

sebhub commented Oct 31, 2019

I used a hack in some high level tests and copy the files from *.txt to *.yml in a temporary directory:

https://github.com/doorstop-dev/doorstop/blob/develop/doorstop/cli/tests/tutorial.py#L159

@stanislaw
Copy link
Member Author

I used a hack in some high level tests and copy the files from *.txt to *.yml in a temporary directory:

This is not possible because in the future there will be tests that you want to run against pre-created setups of Doorstop's .yml files. So while moving everything to tmp would solve this particular PR, the more complex test cases would be great to have pre-created in Input folders. See this for example: https://github.com/doorstop-dev/doorstop/pull/431/files#diff-e99492868d4652cf6c6e268b6408096fR2.

@stanislaw
Copy link
Member Author

This PR is now ready for review.

If you want, I can add more tests in this PR or rather start adding more of them in the following PRs, including my own PR about multiple references.

One detail which I would like you to check in case if you have a Linux machine: please run make test-lit locally and to see if you have any issues. The FileCheck binary is intentionally built on the rather old version of CentOS for binary compatibility and is known to work on both Ubuntu and CentOS systems. Nevertheless, I would like to know if it can cause problems on any other Linux machines.

@sebhub
Copy link
Member

sebhub commented Nov 4, 2019

I think that integration tests which end-to-end test the doorstop functions (doorstop command to file content and version control system state) would be definitely useful. There should be some documentation how to use this new test approach. For example, which steps are necessary to add a new test case.

I am strongly against adding binary programs to the repository. You never know what you get. An open source project repository should be completely transparent to the user.

@stanislaw
Copy link
Member Author

I think that integration tests which end-to-end test the doorstop functions (doorstop command to file content and version control system state) would be definitely useful. There should be some documentation how to use this new test approach. For example, which steps are necessary to add a new test case.

Given that the approach is approved, the docs will follow.

I am strongly against adding binary programs to the repository. You never know what you get. An open source project repository should be completely transparent to the user.

I also don't like it. I have asked about this last week on the LLVM Dev forum already because in our other project we would also prefer to have a non-binary version: [llvm-dev] FileCheck: standalone version? Or how to create a portable binary?.

I am very close to starting a Python version of FileCheck because it seems natural to have both lit and FileCheck to be packaged with pip.

@stanislaw
Copy link
Member Author

I have checked how easy it would be to implement a Python version of FileCheck. It should be rather easy to implement most of it but will probably take some time to implement a 1-1 version. Given that the binaries are no go, I will close this PR for the time being.

LLVM forum is silent, but I will ping them again in a few days.

@stanislaw
Copy link
Member Author

stanislaw commented Nov 29, 2019

Just a heads up here: I have implemented a Python 1-1 port of the FileCheck here: https://github.com/stanislaw/FileCheck.py. I haven't done the implementation of the LLVM-specific features but the main features are done and tested.

This weekend I am going to add the documentation and probably also create a pip package.

Then when I am sure enough that my port behaves without any issues, I can revisit this PR.

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.

3 participants