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

Issue 10992 - Missing unittests when using getUnittests trait #2544

Merged
merged 2 commits into from Sep 30, 2013
Merged

Issue 10992 - Missing unittests when using getUnittests trait #2544

merged 2 commits into from Sep 30, 2013

Conversation

ghost
Copy link

@ghost ghost commented Sep 9, 2013

{
for (size_t i = 0; i < symbols->dim; i++)
{
Dsymbol *symbol = (*symbols)[i];
UnitTestDeclaration *unitTest = symbol->unittest ? symbol->unittest : symbol->isUnitTestDeclaration();
UnitTestDeclaration *unitTest = symbol->isUnitTestDeclaration();
Copy link
Contributor

Choose a reason for hiding this comment

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

Only isUnitTestDeclaration() is not enough. Sometimes it will return null where symbol->unittest won't. If I recall correctly that happens when an UDA is attached to a unit test. I'm pretty sure this is covered in the DMD unit tests.

@ghost
Copy link
Author

ghost commented Sep 10, 2013

Will fix up soon.

@ghost ghost closed this Sep 10, 2013
@ghost ghost reopened this Sep 15, 2013
@ghost
Copy link
Author

ghost commented Sep 16, 2013

What was failing before is that I forgot to rename unittest to ddocUnittest at the call sites. It seems the tests are going green now, although I've added this call: Dsymbols *decl = attrDecl->include(NULL, NULL); This call is made almost everywhere attributes are used, so I've added it here as well.

@jacob-carlborg: The tester seems to be going towards green, is there a specific test where using symbol->isUnitTestDeclaration(); didn't work for you?

@jacob-carlborg
Copy link
Contributor

The tester seems to be going towards green, is there a specific test where using symbol->isUnitTestDeclaration(); didn't work for you?

Those with UDA's if I recall correctly.

@ghost
Copy link
Author

ghost commented Sep 16, 2013

Well it's green now. :)

{
Dsymbols *decl = attrDecl->include(NULL, NULL);
collectUnitTests(decl, uniqueUnitTests, unitTests);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding one more test for this change would be better. Eg.

version(none)
{}
else
{
    unittest { }
    unittest { }
    unittest { }
}

void main()
{
    static assert(__traits(getUnitTests, mixin(__MODULE__)).length == 3);
}

@9rnsr
Copy link
Contributor

9rnsr commented Sep 30, 2013

@AndrejMitrovic Could you please collect the field renaming (unittest -> ddocUnittest) into the first commit? Separating commits based on those logical purposes would be much better.

@9rnsr
Copy link
Contributor

9rnsr commented Sep 30, 2013

Other than nitpicks, LGTM.

@ghost
Copy link
Author

ghost commented Sep 30, 2013

Separated out logical commits and added another test. But please wait for green.

@ghost
Copy link
Author

ghost commented Sep 30, 2013

P.S. for anyone else new to editing specific commits, I've found this SO answer useful: http://stackoverflow.com/a/1186549/279684

Apparently the ^ part at the end is significant too.

@9rnsr
Copy link
Contributor

9rnsr commented Sep 30, 2013

Thanks for the quick response.

9rnsr added a commit that referenced this pull request Sep 30, 2013
Issue 10992 - Missing unittests when using getUnittests trait
@9rnsr 9rnsr merged commit efe4275 into dlang:master Sep 30, 2013
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