Skip to content

Conversation

@cbezault
Copy link
Contributor

@cbezault cbezault commented Oct 23, 2020

This change is mostly cleanups, though it brings us more in line with libcxx's testing format in terms of style and functionality.

I also tried to decrease the size of the TestConfig and STLTest objects as much as possible since those are pickled and communicated to the worker processes via IPC. On my machine I saw a ~20% decrease in testing time with this change (~75mins to ~60mins), we'll see if that holds true on the test machines.

@cbezault cbezault added the test Related to test code label Oct 23, 2020
@cbezault cbezault self-assigned this Oct 23, 2020
@cbezault cbezault changed the title test: let's rewrite the test harness Let's rewrite the test harness Oct 23, 2020
@cbezault cbezault requested a review from rpjohnst October 23, 2020 15:49
@cbezault
Copy link
Contributor Author

@rpjohnst If you could make sure I didn't break your edg_drop machinery that would be much appreciated.

@CaseyCarter CaseyCarter linked an issue Oct 23, 2020 that may be closed by this pull request
@cbezault cbezault marked this pull request as ready for review October 23, 2020 17:46
@cbezault cbezault requested a review from a team as a code owner October 23, 2020 17:46
cbezault and others added 2 commits November 2, 2020 17:10
Co-authored-by: Stephan T. Lavavej <stl@nuwen.net>
Co-authored-by: Stephan T. Lavavej <stl@nuwen.net>
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Nov 3, 2020

I encountered one test failure because my repo root, S:\GitHub\STL, combined with an exceptionally long libcxx path, combined with /analyze's emission of a long-named XML file, exceeded the MAX_PATH limit. This made me realize that /analyze is emitting XML files that we never, ever want. I've pushed a commit to change all occurrences of plain /analyze to /analyze:autolog- which will still emit the warnings we want, but not the XML files. With this change, everything passes for x86:

Testing Time: 6095.88s
  Skipped          :   358
  Unsupported      :  1895
  Passed           : 22678
  Expectedly Failed:  1015

Same for x64:

Testing Time: 5919.71s
  Skipped          :   358
  Unsupported      :  3247
  Passed           : 21326
  Expectedly Failed:  1015

This doesn't change our /analyze:only options. I'll file an issue about that.

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.

I'm totally breaking @StephanTLavavej's 17-attack PR merge combo. 🐼

cbezault and others added 2 commits November 5, 2020 12:00
Co-authored-by: Casey Carter <cartec69@gmail.com>
Co-authored-by: Casey Carter <cartec69@gmail.com>
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.

All good now!

StephanTLavavej added a commit to StephanTLavavej/STL that referenced this pull request Nov 5, 2020
@StephanTLavavej StephanTLavavej merged commit 9ee5529 into microsoft:master Nov 6, 2020
@StephanTLavavej
Copy link
Member

Thanks for this overhaul, improving test performance! This will both increase maintainer/contributor productivity 🚀 and decrease our costs 🤑.

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

5 participants