Skip to content

Conversation

@cbezault
Copy link
Contributor

@cbezault cbezault commented Feb 20, 2020

Description

This PR adds test coverage. Much of the python code in this PR is based off of, or taken verbatim from, code found in llvm-project/libcxx/test.

After configuring and building, running ctest from the build root will run the libcxx and std tests.

Subsets of the tests can be run using the llvm-lit.py script found under <build_root>/tests/llvm-lit/lit.py.
For example, python3.8 <build_root>/tests/llvm-lit/lit.py <source_root>/tests/std/tests/VSO_0000000_any_calling_conventions, will run all tests found under that directory. In the std testsuite there is only a single test under each named directory. In the libcxx testsuite there can be many.

TODO

  • Determine if it is worth having an expected_results.txt and how that would interact with the legacy test harness.

Checklist

Be sure you've read README.md and understand the scope of this repo.

If you're unsure about a box, leave it unchecked. A maintainer will help you.

  • Identifiers in product code changes are properly _Ugly as per
    https://eel.is/c++draft/lex.name#3.1 or there are no product code changes.
  • The STL builds successfully and all tests have passed (must be manually
    verified by an STL maintainer before automated testing is enabled on GitHub,
    leave this unchecked for initial submission).
  • These changes introduce no known ABI breaks (adding members, renaming
    members, adding virtual functions, changing whether a type is an aggregate
    or trivially copyable, etc.).
  • These changes were written from scratch using only this repository,
    the C++ Working Draft (including any cited standards), other WG21 papers
    (excluding reference implementations outside of proposed standard wording),
    and LWG issues as reference material. If they were derived from a project
    that's already listed in NOTICE.txt, that's fine, but please mention it.
    If they were derived from any other project (including Boost and libc++,
    which are not yet listed in NOTICE.txt), you must mention it here,
    so we can determine whether the license is compatible and what else needs
    to be done.

@StephanTLavavej StephanTLavavej added the test Related to test code label Feb 21, 2020
@cbezault
Copy link
Contributor Author

Some high-level comments.

  1. This is still a work in progress as far as cleanup goes. I didn't write a fair amount of the code in this PR and optimistically just took it from libcxx.
  2. I have left the llvm headers in every file I took from llvm where the headers were pre-existing. I need to double check they didn't have any files without headers.

Thanks for taking the time to do this first pass review guys. I'll work through your comments.

@barcharcraz
Copy link
Contributor

Some high-level comments.

  1. This is still a work in progress as far as cleanup goes. I didn't write a fair amount of the code in this PR and optimistically just took it from libcxx.
  2. I have left the llvm headers in every file I took from llvm where the headers were pre-existing. I need to double check they didn't have any files without headers.

Thanks for taking the time to do this first pass review guys. I'll work through your comments.

Yeah, I think a lot of my comments were on things that were pre-existing in llvm code. Depending on how we want to maintain this code and how much we're willing to let it drift from "upstream" we may not want to address all of my concerns :)

Make llvm-lit manually usable in a sane fashion
Address first round of PR comments
@cbezault
Copy link
Contributor Author

@BillyONeal I think the last thing I need is python3 on the CI vms.

Copy link
Contributor

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

You seem to have inadvertently moved the LLVM reference back in time.

EDIT: This has been resolved, but since it was the "final" comment in a review, there's no way to mark it as such.

@BillyONeal BillyONeal mentioned this pull request Mar 24, 2020
4 tasks
@cbezault cbezault requested a review from barcharcraz March 25, 2020 17:36
@cbezault cbezault merged commit 20f21b2 into microsoft:master Mar 25, 2020
@miscco
Copy link
Contributor

miscco commented Mar 26, 2020

Hooray!

Great work @cbezault 🎉

@miscco
Copy link
Contributor

miscco commented Mar 26, 2020

That said somebody wanted to add descriptions to the Readme.md on how to run the tests locally 😇

@cbezault
Copy link
Contributor Author

I'm planning on tackling that tomorrow.

@cbezault
Copy link
Contributor Author

@miscco Didn't get to it today (sorry), dealing with other things and infrastructure instability of the CI. Will be at the top of my TODOs for tomorrow.

@miscco
Copy link
Contributor

miscco commented Mar 27, 2020

@cbezault Dont worry, that was just a friendly banter. I thought working with @CaseyCarter makes you immune to that ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test Related to test code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GitHub: Decide on and create test harness GitHub: Onboard libcxx tests GitHub: Onboard std tests

6 participants