-
Notifications
You must be signed in to change notification settings - Fork 363
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
allow to specify absolute paths for reports. to support msbuild runner #683
allow to specify absolute paths for reports. to support msbuild runner #683
Conversation
@@ -187,6 +196,10 @@ protected String getStringProperty(String name, String def) { | |||
} | |||
|
|||
for (String relPath : relPaths) { | |||
CxxUtils.LOG.debug("Report '{}'", relPath); | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmecosta it's iterating twice over the paths now? Is there a reason for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will take this, left for debugging. forgot to remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@guwirth would you be ok to test this?
53daaa4
to
82097da
Compare
comments done |
@jmecosta will try to test it this week |
Related to #425 |
@jmecosta not sure if I understand the feature in the right way? Following it's not working; sonar-project.properties at D:\Sonar\Test sonar.cxx.other.reportPath=D:/xxx/reports/externalrules-*.xml log:
|
I will change it, don't think there any reasons not to use reports outside On Sun, Nov 29, 2015, 00:43 Günter Wirth [email protected] wrote:
|
Still a problem:
|
@jmecosta Think would be better you write some unit tests. In my example above this should work:
inside root absolute and relative inside root
outside root absolute and relative outside root
outside root on other drive
|
DirectoryScanner scanner = new DirectoryScanner(); | ||
String[] includes = new String[1]; | ||
includes[0] = reportPath; | ||
scanner.setIncludes(includes); | ||
String baseDirPath = baseDirPath1; | ||
scanner.setBasedir(new File(baseDirPath)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is old baseDirPath
@guwirth i will write a integration test, i think its a better solution than unit test for this case. anyway this is still incomplete. thanks for the initial test, i will update and create the it and let you know its ready |
@guwirth do you know why we have 2 basedir paths in the getReports function in CxxReportSensor? can get the use for 2 paths from the tests.. edit: ok, so one is the root of the project and the other is the root of the module |
2275078
to
6b2fd4c
Compare
@jmecosta will test and if positive merge it this evening. |
@jmecosta I'm sorry but my example is still not working 👎
|
Ok suppose we need the it after all. On Tue, Dec 1, 2015, 23:06 Günter Wirth [email protected] wrote:
|
@guwirth actually it almost works. DEBUG: Processed report file 'e:\reports\reports-xunit*.xml' if path is real file then is able to read it. need to figure out how that directory scanner works with these cases. |
@guwirth i am really surprised with the behaviour of this directory scanner... i wrote a test to check for existing paths... the log shows this: 18:58:10.808 [main] DEBUG CxxPlugin - Unprocessed root directory 'E:\src\data' In test it asserts that When i run the analysis i get: DEBUG: Using pattern 'E:\reports\reports-rats*.xml' to find reports Exactly the same output, one detects 1615 reports the other one 0. What i am missing here? Can you cofirm this behaviour in your side? the test is this testWithRealDataInTest this solution should work, as per test. i am ensure why its not working |
drop directory scanner and use http://wilddiary.com/list-files-matching-a-naming-pattern-java/ |
are you suggesting to remove the reactor. for absolute paths the reactor does not apply, the base dir should be basepath or the padron is defined. |
@guwirth i have something that works now for your case, hopefully it works also for other cases but the implementation is now totally different and tests need to be adjusted a lot. also in my opinion we should isolate the tests for path lookup, reports parsing and metrics saving (there should not be any unit tests really but integration tests), having unit tests that do all requires loads of maintenance if we need to refactor some parts as we are doing it now wdyt? |
the implementation is like this: absolute Path: x:\abc\dfe\abc.xml absolute Path: c:\abc\dfe*.xml (or anytype of https://docs.oracle.com/javase/tutorial/essential/io/fileOps.html#glob) relative Paths: abc.xml relative Paths: *.xml (or anytype of https://docs.oracle.com/javase/tutorial/essential/io/fileOps.html#glob) relative Paths: **/abc.xml the code is much simpler now. also a notice the normalizepath function is really slow and we need to find some alternative also during this refactoring |
@jmecosta Travis build is working again. You can squash and rebase to get new results. |
e4f717b
to
3e4f489
Compare
@guwirth its done, but this is not expected to pass for now. ive disable many unit tests. if we agree on a way to go then i will refactor the tests also however i think you can test this to see if it supports your use case |
@guwirth appveyour down? |
@jmecosta Travis & AppVeyor failing, problem in multi-module IT. |
1f3afb3
to
2e2012e
Compare
appveyour should now pass. i need to check that it once it runs again. |
6c89e46
to
4eae78e
Compare
@guwirth i added a new it test, its a complex multimodule project. it can be used to test the process of reports relative to module. its currently testing reports outside project folder, however you might need to create a python function to change the contents of the reports if we are in windows or linux |
5f09764
to
a663b63
Compare
@guwirth tests are now passing in windows, and ITs are also passing in all configurations. there is 1 test failing in unix only that i am ensure whats the issue. not sure why it fails. ive push a commit to improve the logging of the test. i supose review is ready, and please try also running with with your test data that was failing earlier |
All green now |
@jmecosta it's working now 👍
sonar.cxx.other.reportPath=D:/xxx/reports/externalrules-*.xml
sonar.cxx.coverage.reportPath=D:/xxx/reports/coverage-*.xml <?xml version="1.0"?>
<results>
<error file="D:\Sonar\Test\error_recovery\pi\Pi.cpp" line="11" ... />
<error file="d:\sonar\test\error_recovery\pi\pi.cpp" line="12" ... />
</results> |
@@ -0,0 +1,2 @@ | |||
sonar.projectName=2 | |||
sonar.sources=./ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is sources this ./ and not only . ?
89262b0
to
d2038d6
Compare
done |
@Bertk what is your main use case:
|
…te-paths-in-report allow to specify absolute paths for reports. to support msbuild runner
|
support absolute path of report paths