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

Remove trait assignment via regexes #94

Closed
csoltenborn opened this issue Jan 4, 2017 · 13 comments
Closed

Remove trait assignment via regexes #94

csoltenborn opened this issue Jan 4, 2017 · 13 comments
Assignees
Milestone

Comments

@csoltenborn
Copy link
Owner

Nobody seems to be using this feature (see #75). Removing due to simplicity of configuration and code...

@csoltenborn csoltenborn self-assigned this Jan 4, 2017
@csoltenborn csoltenborn added this to the 0.9 milestone Feb 12, 2017
@gobater
Copy link

gobater commented Feb 23, 2017

Could you please update the documentation in README.md?

@csware
Copy link

csware commented Feb 23, 2017

Too bad that this feature was removed - I use it. Why did you not deprecate it and put the poll in the official release notes?

@csoltenborn
Copy link
Owner Author

@gobater Thanks for the reminder!

@csoltenborn
Copy link
Owner Author

@csware Guess what we've done! ;-) See release notes of v0.7.1... I still feared that this would happen.

How bad is your pain? Can you live with providing traits via the C++ macros?

@csware
Copy link

csware commented Feb 23, 2017

I tried the Macros but those seemed to me more painful (also as the provided traits file is outdated for latest gtest).

@csoltenborn
Copy link
Owner Author

I see... I will discuss with Jonas what to do about that, but no promises. Note that we will soon add support for gtest 1.8.0 - in the meantime, you can look up the (very simple) change to our macro file here (which is, as I just realized, your very own pull request - note that all the things I suggested to do in that pull request are not actually needed to make use of them!)...

@csoltenborn csoltenborn reopened this Feb 23, 2017
@csoltenborn
Copy link
Owner Author

@csware We are going to re-add trait assignment by regexes. The new release is going to happen next week...

csoltenborn added a commit that referenced this issue Feb 24, 2017
@csoltenborn
Copy link
Owner Author

@csware This build has support for trait assignment via regexes again.

@gobater
Copy link

gobater commented Feb 27, 2017

@csoltenborn I am happy of this feature coming again :-)

I mainly see two problems of the trait assignment via macros:

  • The header file needs to be adapted / fixed for each new release of google test
  • The sources compile (most probably) only with MSVC compiler --> clearly traits make sense only in VS

I am currently using VS editor and compiler to develop my SW, but I am considering switching to GCC in the future because of its strong C++ std support. I guess in that case, compilation of my test code could be broken. Therefore, regexes is a valid alternative when working with VS.

I do not know if it is possible, but would be interesting if something similar to GoogleTest Properties could be used via a macro that could be easily disabled via preprocessor switch...
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.md#logging-additional-information

Nevertheless... Thank you for your support

@csoltenborn
Copy link
Owner Author

If you switch to a new release of Google Test, you anyways have to do all kinds of changes, so does it really matter if you have to update one more file? After all, that file is provided by us...

Our macro file should compile just fine with other compilers than the MS one - in fact, we only add some methods to the generated test classes and encoded the traits into these methods (which are never called). However, switching to a different compiler will break at least parts of GTA - we use DIA (the MS API for accessing pdb files) to receive source file locations of tests as well as traits encoded via our macros. As long as the compiler does not produce pdb files parseable by DIA (and I doubt that any compiler does), this information will not be available.

Note that we could quite easily abstract the "receive source file locations and traits" part of the GTA code and add the ability to provide different implementations for different compilers, but the actual implementations would most probably need to be contributed, so I guess we are only going to do this if there's actual interest.

As far as I can see, using the RecordProperty() option only provides additional information in the generated XML test result file. This is not good enough for us: For the sake of greater interactivity, we parse the executable's output during test execution, and report test results back to Visual Studio as soon as they are available. However, we considered using a TestEventListener to receive test run information from Google Test but this has a couple of drawbacks, the worst being that test code would have to be changed before GTA could be used. Therefore, we sticked with output-based results parsing (using the XML test result file as a fallback).

@gobater
Copy link

gobater commented Feb 27, 2017

Concerning the portability issue, I am worried about the usage of "__declspec(dllexport)" which only the Ms-Windows version of GCC supports. I am aware that switching to another compiler could break GTA (pdb xxx), but in my opinion it shall not break the compilation of the code (which currently is the case). In case that that solution is preferred, it shall be possible to enable/disable the feature (switch to the "general" GTEST macros) by means of a preprocessor define (ie. -DGTA_ENABLE_TRAITS=FALSE)

RecordProperty only provides information in the XML result file. I do not know any insides of GTA and VS, but there are some other VS extensions which parse the sources, Additionally, maybe it could be done by means of a preprocessor trick.

With my comment I didn't wanted to extend the scope of this enhancement, just wanted to mention some issues of the macro and "justify" why regexes can be an option.

@csoltenborn
Copy link
Owner Author

Oops - I had forgotten about the __declspec(dllexport) stuff... The sole purpose of that is to avoid that the methods are optimized away - it would maybe be possible to achieve this in a more compiler independent way.

We are probably not going to parse the source code, mostly due to performance concerns. What kind of preprocessor trick do you have in mind?

Thanks for your comments anyway!

@peterbudai
Copy link
Contributor

peterbudai commented May 2, 2017

I just stumbled upon this issue. We're using solely regex based traits discovery, as our project is quite big and produces very large binaries that cannot really be parsed (see #41). Using the macros is not an option to us, since we're building for not just VS but 5 other platforms, so I was really glad to see this feature back.

csoltenborn pushed a commit that referenced this issue Aug 4, 2018
* add null check for native method that returns null on fail

* move null check out of loop

* add an error to the log on failure and add a null check in the other place it's used as well
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants