-
-
Notifications
You must be signed in to change notification settings - Fork 236
Refactor integration tests #3064
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
base: master
Are you sure you want to change the base?
Conversation
✅ PR OK, no changes in deprecations or warnings Total deprecations: 0 Total warnings: 0 Build statistics: statistics (-before, +after)
executable size=5265672 bin/dub
rough build time=59s Full build output
|
604aa21
to
62d6498
Compare
CI looking good, outside of buildkite which I don't have access to |
|
Simply removing that line and changing whatever was used to invoke the test runner with Although, that test is one of the more intrusive ones. It used to overwrite I did get a few failures when running the testsuite as part of a Gentoo build and one of it seems to be a genuine (minor) bug in the vibe json code but I need more time to make sure it's an actual bug, because I can only reproduce it with a locally built ldc2, and not with dmd, gdc or the upstream releases of ldc2. |
The dlang/ci error is https://github.com/dlang/ci/blob/dc95e2fe07e163e159ae3b5b2bf1efc374fef818/buildkite/build_project.sh#L180-L184 Can get around it by adding |
Oh, so that's where it was.
buildkite doesn't look like it's running the integration tests, so what's the point in removing those unused test files? Should buildkite be running the full testsuite? |
In regards to the bug: dub/source/dub/internal/vibecompat/data/json.d Line 1452 in ad3f246
Json.this(BigInt)
zeroFields and then initBigInt dub/source/dub/internal/vibecompat/data/json.d Line 1264 in ad3f246
BigInt.opAssign that, in turn, calls BigUint.opAssign . BigUint , however, has an invariant :https://github.com/dlang/phobos/blob/2d8ae67b396f5cc5f4633695c1c5ac67d2bf448e/std/internal/math/biguintcore.d#L240-L244: pure invariant()
{
assert( data.length >= 1 && (data.length == 1 || data[$-1] != 0 ),
"Invariant requires data to not empty or zero");
} which then fails because the data array has been zero-ed out by
You can hit this if the stdlib has been built without
This shouldn't be causing any problems though, since the bigint would be in a valid state after |
See dlang/dub#3064 (comment) 1. It does nothing 2. It causes the run to fail when the tests are renamed/removed Signed-off-by: Andrei Horodniceanu <[email protected]>
This changes makes the compiler version for gdmd match the gdc version instead of the dmd FE version that gdc had been using. This matches what happens for ldmd2 which mirrors the ldc2 version. Specifically, now the versions look like: - ldmd2-1.40 :: 1.40.X - ldc2-1.40 :: 1.40.X - gdc-14 :: 14.X.Y - gdmd-14 :: 14.X.Y When, previously, gdmd-14 would result in a version like 2.108.X Signed-off-by: Andrei Horodniceanu <[email protected]>
62d6498
to
55ec9c7
Compare
Incorporated #3066 and changed I've also updated the tests' |
As a small benchmark, doing two consecutive runs, the first with a clean repo and no
|
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
Signed-off-by: Andrei Horodniceanu <[email protected]>
ldc2 on alpine is configured to use the lld linker, however, the lld package is not pulled in as a dependency. Signed-off-by: Andrei Horodniceanu <[email protected]>
55ec9c7
to
523afd7
Compare
Changed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would like to get this in soon (DConf hackathon perhaps?). Why the move the new_test though ?
// Find the backend version of the compiler, not the dmd FE. | ||
// Specificically, for gdmd-14 this function should return | ||
// 14.X.Y not 2.108.Z |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why ? I don't think that makes a lot of sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #3066 (comment).
For short:
ldmd
uses the version ofldc
, not the dlang frontend version- I've only seen people depend on major versions of gcc, at most a minor version if they need a bug fix. Having versions like
2.108.1
,2.108.0-rc.1
etc. all map togdc-14
seems like poor design.
I feared that trying to do it in place would make it way harder to do atomic commits for easier review and I also worried that I would miss a dependency on one of the tests. Putting everything in another directory helped with safely porting tests one by one. This does leave me with having to put everything back into tests. I'm fine doing it in 1 big commit but I think I can manage rebasing each commit to leave everything in |
Main takeaways
gdc
.sh
tests with.d
programs (in order to run on windows)It took me a while but I think it was worth it.
I've kept the commits to be one test each and the changes are staged to a new directory (
new_tests
). I don't expect this to be the final form if this PR is merged so I'll keep this as a draft but I hope that this would make reviewing easier.From my notes while working on this:
frameworks
wasn't testing anything (Add ability to specify frameworks to link against #3061 (comment))issue2046
wasn't runissue2650-deprecated-modules
wasn't runissue103
is missing a test case (https://github.com/dlang/dub/pull/1177/files#diff-a6b4cf5ac30d4b153861d8ad368ab9cb2aebf25ee28482a6ec201a827ee25a98)ddox
sometimes decided to ask for confirmation to overwrite files from stdin on windows (making the test stuck). Maybe it had to do with my setup in which I was making changes to how it was run so maybe it's unlikely that a real user would actually hit it. Regardless, I thinkdub
should be preventing the spawned process for having access tostdin