Allow @betterC tests in Phobos by wilzbach · Pull Request #6645 · dlang/phobos

wilzbach · 2018-07-31T16:48:11Z

Open questions:

Should we remove the test/betterC folder? (the @BetterC test extraction is imho a better replacement)

Requires: dlang/tools#357

dlang-bot · 2018-07-31T16:48:12Z

Thanks for your pull request, @wilzbach!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + phobos#6645"

PetarKirov · 2018-07-31T16:51:55Z

I guess having a separate folder is good primary for integration tests or test code that doesn't fit into any particular std.* module.

wilzbach · 2018-07-31T16:51:00Z

.circleci/run.sh

    fi

-    for proj in dmd druntime ; do
+    for proj in dmd druntime tools ; do


The auto-tester and other CI behave like this already anyhow.

(Also locking tools to a specific version was done initially because it seemed as a good idea at the time, but in practice it just made updates more annoying.)

wilzbach · 2018-07-31T16:51:30Z

posix.mak

+################################################################################
+
+betterc-phobos-tests: $(addsuffix .betterc,$(D_MODULES))
+betterc: betterc-phobos-tests betterc-run-tests


We could add an alias betterC: betterc if wanted.

wilzbach · 2018-07-31T16:52:32Z

posix.mak

+		-defaultlib= -debuglib= $(LINKDL) $<
+	./$(ROOT)/unittest/betterC/$(notdir $(basename $<))
+
+betterc-run-tests: $(subst .d,.run,$(wildcard test/betterC/*.d))


There's probably not much value in keeping these separate tests anymore as @betterC is almost as powerful and we avoid lots of redundant declarations.

Yep, I'm ok with removing this target.

wilzbach · 2018-07-31T16:55:06Z

std/algorithm/comparison.d


 ///
-@safe unittest
+@safe @betterC unittest


This won't be displayed on dlang.org yet, but the next step would be to figure out how to get Ddoc / Ddox to do so.
(it will be a lot easier for Ddox though.)

wilzbach · 2018-07-31T17:00:01Z

Should we remove the test/betterC folder? (the @BetterC test extraction is imho a better replacement)

I guess having a separate folder is good primary for integration tests or test code that doesn't fit into any particular std.* module.

Yep, but do we have examples where we would actually need to do this for Phobos?
AFAICT the main goal is just to ensure that most of Phobos eventually works with -betterC and I think the @betterC works well in this regard. For everything else, I guess DMD's testsuite is better suited?

andralex

LGTM, feel free to zero in on the details in review. Thanks Seb!

wilzbach · 2018-08-14T14:37:41Z

With dlang/tools#357 merged, this should be green now :)

WalterBright · 2018-08-14T23:01:04Z

std/internal/attributes.d

@@ -0,0 +1,4 @@
+module std.internal.attributes;
+
+// Internal attributes to be to e.g. annotate unittest blocks


What does this attribute mean? That it only works with betterC, or that it works with regular D or betterC?

PetarKirov · 2018-08-15T05:13:32Z

@WalterBright, the intention is similar to other languages attributes like pure - @betterC unittests need to execute successfully in normal D and with -betterC.

PetarKirov · 2018-08-14T14:41:34Z

std/internal/attributes.d

@@ -0,0 +1,4 @@
+module std.internal.attributes;
+
+// Internal attributes to be to e.g. annotate unittest blocks


/** Used to annotate `unittest`s which need to be tested in a `-betterC` environment. Such `unittest`s will be compiled and executed without linking druntime in, with a `__traits(getUnitTests, mixin(__MODULE__))` style test runner. Note that just like any other `unittest` in phobos, they will also be compiled and executed without `-betterC`. */ package(std) enum betterC = 1;

PetarKirov · 2018-08-15T08:23:49Z

posix.mak

+test/betterC/%.run: test/betterC/%.d $(DMD) $(LIB)
+	mkdir -p $(ROOT)/unittest/betterC
+	$(DMD) $(DFLAGS) -of$(ROOT)/unittest/betterC/$(notdir $(basename $<)) -betterC $(UDFLAGS) \
+		-defaultlib= -debuglib= $(LINKDL) $<


-defaultlib= -debuglib= -> $(NODEFAULTLIB)

PetarKirov · 2018-08-15T08:26:35Z

posix.mak

+		-defaultlib= -debuglib= $(LINKDL) $<
+	./$(ROOT)/unittest/betterC/$(notdir $(basename $<))
+
+betterc-run-tests: $(subst .d,.run,$(wildcard test/betterC/*.d))


Yep, I'm ok with removing this target.

PetarKirov · 2018-08-15T08:29:29Z

posix.mak

+DUB_PATH=$(abspath $(shell which $(DUB)))
+ifeq (,$(DUB_PATH))
+	DUB_PATH=$(DUB_DIR)/bin/dub
+	DUB=$(DUB_DIR)/bin/dub


DUB=$(DUB_PATH) ?

PetarKirov · 2018-08-15T08:30:44Z

posix.mak

+endif
+
+$(DUB): | $(DUB_DIR) $(LIB)
+	cd $(DUB_DIR) && DFLAGS="$(DFLAGS) $(LIB) $(NODEFAULTLIB) $(LINKDL)" DMD=$${PWD}/$(DMD) ./build.sh


Why is $(NODEFAULTLIB) necessary here?

Ah, I guess because you want to use the version of druntime and phobos that were just built.

Well, I have to build dub from source, because auto-tester doesn't come with a dub version (kinda silly) and also no globally installed dmd (HOST_DMD is only exposed when building dmd and my PR to fix that has remained unanswered: braddr/at-client#9).

wilzbach · 2018-08-16T09:47:39Z

This is sadly blocked until I figure out a reliable way to use or build dub on the auto-tester.
At the moment, cloning dub and building it seems to work, but due to make's parallel execution this seems to happen in many parallel steps and some of them fail :/

wilzbach · 2018-08-16T09:58:09Z

This is sadly blocked until I figure out a reliable way to use or build dub on the auto-tester.

Ah well, I want to start adding @betterC test and stop regressions, so I removed the last experimental commit that adds dub to the auto-tester (and will move this into a new PR) and just enabled the betterC on CircleCi.
This is a bit problematic, because the CircleCi tests aren't run on dmd nor druntime, but thanks to our new Buildkite setup I will later today setup Buildkite, s.t. it also runs all testsuites and we can prevent such problems.

Buildkite for now.

wilzbach · 2018-08-21T15:22:19Z

This is a bit problematic, because the CircleCi tests aren't run on dmd nor druntime, but thanks to our new Buildkite setup I will later today setup Buildkite, s.t. it also runs all testsuites and we can prevent such problems.

With #6669 and dlang/ci#287 merged, Buildkite will now run the buildkite-test target on every PR.
This means that the @betterC tests added here will also be run on every PR at dmd, druntime and of course Phobos and thus I think we can finally move on with this (:

wilzbach · 2018-08-21T15:23:36Z

posix.mak

+		$(NODEFAULTLIB) $(LINKDL) $<
+	./$(ROOT)/unittest/betterC/$(notdir $(basename $<))
+
+betterc-run-tests: $(subst .d,.run,$(wildcard test/betterC/*.d))


As mentioned in earlier discussions, there's no real need to for these separate betterC tests anymore and I would be ok with removing them either in this PR or a follow-up.

wilzbach requested review from CyberShadow, JackStouffer, MartinNowak, PetarKirov and andralex as code owners July 31, 2018 16:48

wilzbach commented Jul 31, 2018

View reviewed changes

wilzbach force-pushed the unittest-betterC branch from 66f1585 to 35a2d0d Compare July 31, 2018 16:56

wilzbach added the Merge:Blocked label Jul 31, 2018

wilzbach force-pushed the unittest-betterC branch from 35a2d0d to 789b245 Compare July 31, 2018 17:03

wilzbach mentioned this pull request Jul 31, 2018

Support parsing only specific unittests (e.g. @betterc-test) dlang/tools#357

Merged

andralex approved these changes Jul 31, 2018

View reviewed changes

PetarKirov removed the Merge:Blocked label Aug 14, 2018

wilzbach force-pushed the unittest-betterC branch from 789b245 to f6dd1e8 Compare August 14, 2018 14:37

wilzbach added the Merge:auto-merge label Aug 14, 2018

wilzbach mentioned this pull request Aug 14, 2018

Make std.typecons.Tuple betterC compatible #5952

Merged

wilzbach force-pushed the unittest-betterC branch from f6dd1e8 to a9cecac Compare August 14, 2018 15:09

dlang-bot removed the Merge:auto-merge label Aug 14, 2018

wilzbach added the Merge:auto-merge label Aug 14, 2018

dlang-bot removed the Merge:auto-merge label Aug 14, 2018

WalterBright reviewed Aug 14, 2018

View reviewed changes

PetarKirov approved these changes Aug 15, 2018

View reviewed changes

wilzbach force-pushed the unittest-betterC branch 4 times, most recently from dcf15af to 28e322c Compare August 15, 2018 14:40

wilzbach force-pushed the unittest-betterC branch from 28e322c to fda5cba Compare August 15, 2018 15:17

wilzbach added the Merge:Blocked label Aug 16, 2018

wilzbach mentioned this pull request Aug 16, 2018

Expose Dub binary to the Make build scripts braddr/d-tester#73

Open

wilzbach force-pushed the unittest-betterC branch from fda5cba to 8628390 Compare August 16, 2018 09:54

This was referenced Aug 16, 2018

Add a special buildkite Makefile target that can be used to run Phobos testsuite on Buildkite #6669

Merged

Run Phobos's testsuite on the Project Tester dlang/ci#287

Merged

wilzbach force-pushed the unittest-betterC branch 2 times, most recently from 39ed40e to 4fded99 Compare August 16, 2018 13:26

wilzbach added 3 commits August 21, 2018 17:19

Add @BetterC extraction testsuite

1302dd5

Checkout the respective branch of tools

c0f3b26

Start to add @BetterC to std.algorithm.comparison

57d1256

wilzbach force-pushed the unittest-betterC branch from 4fded99 to eb777d5 Compare August 21, 2018 15:19

Don't run betterC tests on the auto-tester and only on CircleCi +

eb777d5

Buildkite for now.

wilzbach commented Aug 21, 2018

View reviewed changes

PetarKirov added betterC Merge:auto-merge and removed Merge:Blocked labels Aug 21, 2018

dlang-bot merged commit f4aec67 into dlang:master Aug 22, 2018

wilzbach mentioned this pull request Sep 20, 2018

Remove separate @betterC tests #6714

Merged

wilzbach deleted the unittest-betterC branch September 20, 2018 16:39

wilzbach mentioned this pull request Aug 1, 2019

Add @betterC to druntime dlang/druntime#2715

Merged

		@@ -0,0 +1,4 @@
		module std.internal.attributes;

		// Internal attributes to be to e.g. annotate unittest blocks

Uh oh!

Comments

Conversation

wilzbach commented Jul 31, 2018

Uh oh!

dlang-bot commented Jul 31, 2018

Bugzilla references

Testing this PR locally

Uh oh!

PetarKirov commented Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Jul 31, 2018

Uh oh!

andralex left a comment

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Aug 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PetarKirov commented Aug 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Aug 16, 2018

Uh oh!

wilzbach commented Aug 16, 2018

Uh oh!

wilzbach commented Aug 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

PetarKirov commented Jul 31, 2018 •

edited

Loading