Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.
/ druntime Public archive

Comments

Add tests for GC.removeRoot#1679

Merged
braddr merged 1 commit intodlang:masterfrom
Burgos:addroot
Jun 7, 2017
Merged

Add tests for GC.removeRoot#1679
braddr merged 1 commit intodlang:masterfrom
Burgos:addroot

Conversation

@Burgos
Copy link
Contributor

@Burgos Burgos commented Oct 21, 2016

Calling removeRoot on something that was not added
by addRoot should be possible.

https://issues.dlang.org/show_bug.cgi?id=9275

@Burgos
Copy link
Contributor Author

Burgos commented Oct 21, 2016

Fails on CI because runner can't download DMD:

Failed to download 'http://nightlies.dlang.org/dmd-2.068.2/LATEST'
+ source ''
./circleci.sh: line 31: : No such file or directory

@WalterWaldron
Copy link
Contributor

Nice, you've added a place for GC unit tests (they used to get put at the bottom of src/rt/lifetime.d.)

@wilzbach
Copy link
Contributor

Fails on CI because runner can't download DMD:

Wow good catch!
This was introduced by dlang/installer#200 (support for building feature branches for dmd). Seems like we should have a bit more eyes on our installation scripts.
I submitted a quick fix: dlang/installer#201

@Burgos
Copy link
Contributor Author

Burgos commented May 9, 2017

Rebased and now the tests are passing. Except the code coverage test, though. However, this just adds tests, and I don't know how it could this affect the rwmonitor.d and thread.d coverage, given that it should at least increase the coverage (not that it's touching any of that).

@Burgos
Copy link
Contributor Author

Burgos commented May 9, 2017

Oh, that actually looks like it's showing the lines that are not covered, not delta. I guess it just has more lines with these tests, so the ratio is smaller.

@rainers
Copy link
Member

rainers commented May 10, 2017

Not a bad idea to start a separate test suite for the GC. There's a problem with the test folder, though: it's never built on Windows.

Similar unittests are currently in src/gc/impl/conservative/gc.d. If you actually care about this test only, it might be better to just add it there.

@Burgos
Copy link
Contributor Author

Burgos commented May 10, 2017 via email

@dlang-bot
Copy link
Contributor

Fix Bugzilla Description
9275 [GC] removeRoot hits assert(0) instead of being a no-op (as documented)

@dlang-bot dlang-bot added the Bug Fix Include reference to corresponding bugzilla issue label May 13, 2017
@Burgos
Copy link
Contributor Author

Burgos commented May 13, 2017

@rainers I've updated the code, so that I've moved the tests just into the unittest in the suggested file. Thanks!

Issue 9275 describes the runtime crashing on the
invalid pointer passed to removeRoot, instead of
documented no-op.

This has been fixed in the meantime, so unittest
is added as a guarantee that it stays so.
@braddr braddr merged commit 2b14f6c into dlang:master Jun 7, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Bug Fix Include reference to corresponding bugzilla issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants