Skip to content
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

Add check for public unittests #4943

Merged
merged 3 commits into from Dec 16, 2016

Conversation

wilzbach
Copy link
Member

This is an effort to revive dlang/dlang.org#1297, which will allow to run unittests directly on dlang.org

The main problem is that for quite many snippets we don't know which imports are needed. The proposed solution is to extract all unittest blocks from Phobos and execute them :)
To ensure that the snippets on dlang.org will be runnable in the future this PR adds a new Makefile target publictests and enables this in the CircleCi setup. I opened a PR at the tools repo for the extraction script: dlang/tools#203 on which this PR obviously depends on.

As a start this PR also contains "fixes" to a couple of modules by adding required local imports to their public unittests.
The idea is to gradually increase the number of supported modules with the CI helping to verify further PRs ;-)
For the list of currently ignored module see the Makefile target.

Copy link
Member

@andralex andralex left a comment

Choose a reason for hiding this comment

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

Terrific work, thanks!

@@ -608,8 +607,8 @@ public:
//named groups are unaffected by range primitives
assert(c["var"] =="a");
assert(c.front == "42");
----
+/
Copy link
Member

Choose a reason for hiding this comment

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

hm, this was not even a unittest...

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I accidentally saw this and thus "ddocified" the example to a unittest

@andralex
Copy link
Member

/bin/sh: 1: ../../tools/phobos_tests_extractor.d: not found

obviously we need to pull the extractor first :)

@wilzbach wilzbach removed the Blocked label Dec 15, 2016
@wilzbach wilzbach force-pushed the add-check-for-public-unittests branch 7 times, most recently from 66c57ad to 7135fbb Compare December 16, 2016 00:12
@wilzbach wilzbach force-pushed the add-check-for-public-unittests branch from 7135fbb to 26ab57d Compare December 16, 2016 00:26
@wilzbach
Copy link
Member Author

obviously we need to pull the extractor first :)

So with the extractor being merged, CircleCi is now passing 🎉

(Note that only a subset of modules are enabled for this check, but as mentioned the idea is to gradually decrease the excluded set)

# compile all public unittests separately
publictests()
{
clone https://github.com/dlang/tools.git ../tools master
Copy link
Member

Choose a reason for hiding this comment

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

Why master instead of $base_branch? I'd argue that unconditionally using master is almost always wrong, b/c it's not tied to the phobos version.
In fact we were only able to resolve most of our testing issues, after scrutinizingly pinning all our dmd, dub, et.al. versions.
Master also means that repeating builds might not be reproducible, that's just asking for troubles (e.g. I spend about 1 week on Issue 16574 b/c the reporter only provided a moving target).
That clone does print the exact commit version, right? So at least it's reproducible.

Possible ways to avoid future issues:

  • checkout a specific version (v2.072.2) or commit of that file in the tools repo
  • maintain that tool in phobos if it's merely designed for phobos
  • do dmd independent releases of that script and pin those

Copy link
Member Author

Choose a reason for hiding this comment

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

Why master instead of $base_branch? I'd argue that unconditionally using master is almost always wrong, b/c it's not tied to the phobos version.

Thanks for the advice! :)

Possible ways to avoid future issues:
checkout a specific version (v2.072.2) or commit of that file in the tools repo

Okay I have done that.

maintain that tool in phobos if it's merely designed for phobos

Well it's general in the sense that the script has nothing about Phobos hard-coded (except for the script's name), but I think that would have been the best option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants