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

Addition of ClojureAnalyzerFactoryTest #1112

Merged
merged 13 commits into from
Jun 14, 2016
Merged

Conversation

fzakaria
Copy link

@fzakaria fzakaria commented Jun 9, 2016

I noticed that when I was running the tests, it did not look like it was analyzing the Clojure code correctly.
I was seeing plenty of errors of the type repetition-operator operand invalid wth the ctags regex provided.

Grokking around, I found similar ctag regex that didn't result in the error and I wrote a test to verify that the definitions are correctly resolving.

I also added Maven Wrapper because I think it is good practice for any project that uses Maven.

FWIW here is my version of ctags:

ctags --version
Exuberant Ctags 5.8, Copyright (C) 1996-2009 Darren Hiebert
Compiled: Sep 13 2015, 03:28:50
Addresses: dhiebert@users.sourceforge.net, http://ctags.sourceforge.net
Optional compiled features: +wildcards, +regex

Farid Zakaria added 4 commits June 9, 2016 09:17
Errors were noted from ctags when trying to analyze clojure files `repetition-operator operand invalid`
Changed the regex matching for the clojure files to remove such errors and added a unit test
for ClojureAnalyzer
https://github.com/takari/maven-wrapper
This will help standardize the version of maven that is necessary to build the project.
Added a test and checked that the Definitions are properly resolving to the correct
tag type and line number.
@tarzanek
Copy link
Contributor

Looks ok but
We need to resolve the license headers to add to new files

Also the sample clj file - it is under cpl license, I need to figure out if it can be merged with cddl.
Other option is to find some other sample clj file under bsd/mit license or write something by yourself.

Also ideally get the oca to Oracle inboxes, I will ping the oca owner for opengrok to handle your signature quickly.

Ttyl
L

@tarzanek tarzanek self-assigned this Jun 10, 2016
@tarzanek
Copy link
Contributor

Also the regexp needs to be posix compliant so ctags on Solaris or other posix system will do correct things

@tarzanek
Copy link
Contributor

And travis fails - https://travis-ci.org/OpenGrok/OpenGrok
since you added new file to sources the generic testing now picks up more definitions (10 instead of 7, you need to verify if it is true + if you change for some other file you need to check and retest it)

fwiw I use universal ctags - https://github.com/universal-ctags/ctags
they are much better

@tarzanek
Copy link
Contributor

and I just recieved your OCA, thank you for that! :)
now we just miss the proper license headers in respective files

@tarzanek
Copy link
Contributor

fwiw - the skipped tests are nicely reported in jenkins test UI :)

@@ -67,6 +67,7 @@
= Pattern.compile("([\\.\\d]+)\\W+\\((\\w+)");

public CVSRepository() {
working = Boolean.FALSE;
Copy link
Author

Choose a reason for hiding this comment

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

this should not default to true (which RCS from it inherits does)

@fzakaria
Copy link
Author

@tarzanek thanks for looking at it. Not sure why I didn't see the travis build earlier.
I tried to fix some errors however its tough to get a "perfect" build on my machine due to the myriad of settings that are configurable (derby not present etc..)

  1. I replaced the clojure file with some home-grown sample
  2. Fixed as many errors as I could that might have been on account of the inclusion of the new clojure file (i.e. Searcher)
  3. Fixed some additional errors I had when running mvn test

Is the jenkins build private ? Travis doesn't show a test JUnit breakdown from the reports.
Also, one of the other developers said they would add junit-force-all to the Ant build - meaning that Jenkins should probably not be reporting skipped tests.

@tarzanek
Copy link
Contributor

@fzakaria sorry for travis, my fault, github introduced new permissions and I forgot to give part of them to travis, now travis should be seen again in pull reqs.
jenkins build is unfortunately private, but I will just add the junit-force-all to ant, for now it's ok like it is

command.add("--regex-clojure=/\\([[:space:]]*defstruct[[:space:]]+([-[[:alnum:]]*+!_:\\/.?]+)/\\1/s,struct/");
command.add("--regex-clojure=/\\([[:space:]]*intern[[:space:]]+([-[[:alnum:]]*+!_:\\/.?]+)/\\1/v,intern/");
command.add("--regex-clojure=/\\([[:space:]]*ns[[:space:]]+([-[[:alnum:]]*+!_:\\/.?]+)/\\1/n,namespace/");
command.add("--regex-clojure=/\\([ \\t]*create-ns[ \\t]+([-[:alnum:]*+!_:\\/.?]+)/\\1/n,namespace/");
Copy link
Contributor

Choose a reason for hiding this comment

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

please use :space: and :alnum: as defined in posix for regexps, don't use " \t"

Copy link
Contributor

Choose a reason for hiding this comment

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

@tarzanek tarzanek added this to the 0.13 milestone Jun 13, 2016
@tarzanek
Copy link
Contributor

tarzanek commented Jun 13, 2016

fwiw - all looks fine, I just don't like that you switch char classes to just few chars, that said clojure might be using the other whitespace so I might be wrong, but still, if possible please make sure the regexps are posix compliant, otherwise your code/ctag def will fail terribly on posix systems such as Solaris (ctags consume posix regexp library there, so they behave slightly different than on linux where they use the gnu version of regexp library)

@fzakaria
Copy link
Author

@tarzanek updated the ctags regex to be POSIX compliant.
(test still passes, which I'm taking as a sign its working - hurray for tests?)

@tarzanek
Copy link
Contributor

thank you, merging, will recheck the tests on the private jenkins (which runs on solaris 12)

@tarzanek tarzanek merged commit 798be88 into oracle:master Jun 14, 2016
@tarzanek
Copy link
Contributor

fwiw - some change is still needed, since tests fail for me on Solaris, so I will just go ahead and commit the necessary posix changes

@tarzanek
Copy link
Contributor

ok, reason is actually that universal ctags has clojure parser which isn't as good as the gist we use
I filed universal-ctags/ctags#988
let's see if they resolve it, until then I will just use clojure from opengroks defs

@fzakaria fzakaria deleted the regex-clojure branch June 15, 2016 16:24
@fzakaria
Copy link
Author

awesome.

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

Successfully merging this pull request may close these issues.

2 participants