Skip to content

Fix error with space before annotation params #375

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Jul 4, 2017
Merged

Fix error with space before annotation params #375

merged 9 commits into from
Jul 4, 2017

Conversation

Pazus
Copy link
Member

@Pazus Pazus commented Jun 28, 2017

addresses #357

@Pazus Pazus added the bugfix label Jun 28, 2017
@Pazus Pazus added this to the v3.0.2 milestone Jun 28, 2017
@Pazus Pazus self-assigned this Jun 28, 2017
@coveralls
Copy link

coveralls commented Jun 28, 2017 •
edited
Loading

Coverage Status

Coverage remained the same at 91.708% when pulling 64cab8e on Pazus:issue-357 into 9b77fda on utPLSQL:develop.

jgebal
jgebal requested changes Jun 29, 2017
View reviewed changes
source/core/ut_suite_manager.pkb Outdated
@@ -84,7 +84,7 @@ create or replace package body ut_suite_manager is
l_suite_name := l_annotation_data.package_annotations('suite').text;
end if;

if l_annotation_data.package_annotations.exists('suitepath') then
if l_annotation_data.package_annotations.exists('suitepath') and trim(l_annotation_data.package_annotations('suitepath').text) is not null then
Copy link
Member

Choose a reason for hiding this comment

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

I think that the trim should be done on the text of annotation param inside the annotation parser.

tests/ut_suite_manager/ut_suite_manager.emptySuitePath.sql Outdated
--act
l_objects_to_run := ut_suite_manager.configure_execution_by_path(ut_varchar2_list('tst_empty_suite_path'));

if ut_expectation_processor.get_status = ut_utils.tr_success then
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand how will this test ever fail, as there is no expectation executed before status is checked.

tests/ut_annotations/ut_annotations.parse_package_annotations.spaceBeforeAnnotationParams.sql Outdated

--Assert
l_ann_param := null;
l_ann_param.val := 'Name of suite';
Copy link
Member

Choose a reason for hiding this comment

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

Lines 24 and 25 are not used I think

source/core/ut_annotations.pkb Outdated
@@ -30,7 +30,7 @@ create or replace package body ut_annotations as
c_rgexp_identifier constant varchar2(50) := '[a-z][a-z0-9#_$]*';
c_annotation_block_pattern constant varchar2(200) := '(({COMMENT#.+}'||chr(10)||')+)( |'||chr(09)||')*(procedure|function)\s+(' ||
c_rgexp_identifier || ')';
c_annotation_pattern constant varchar2(50) := gc_annotation_qualifier || c_rgexp_identifier || '(\(.*?\)$)?';
c_annotation_pattern constant varchar2(50) := gc_annotation_qualifier || c_rgexp_identifier || '[ '||chr(9)||']*(\(.*?\)$)?';
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we use \s* rather than chr(9)?
We should also enhance the pattern to allow \s at the end I think.
So how about:

gc_annotation_qualifier || c_rgexp_identifier || '\s*(\(.*?\)\s*$)?';

Copy link
Member Author

Choose a reason for hiding this comment

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

\s contains new line character which is not allowed

Copy link
Member

Choose a reason for hiding this comment

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

Ok

Copy link
Member

Choose a reason for hiding this comment

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

Can you make the regexp work with trailing spaces?
'[ '||chr(9)||]((.?)[ '||chr(9)||']*$)?'

@coveralls
Copy link

coveralls commented Jun 29, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.2%) to 91.858% when pulling eb0a999 on Pazus:issue-357 into 021eead on utPLSQL:develop.

@Pazus
addressed comments
f2ca323 
fixed additional recent tests
@coveralls
Copy link

coveralls commented Jul 1, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.2%) to 91.858% when pulling f2ca323 on Pazus:issue-357 into 021eead on utPLSQL:develop.

jgebal
jgebal requested changes Jul 2, 2017
View reviewed changes
source/core/ut_annotations.pkb Outdated
@@ -30,7 +30,7 @@ create or replace package body ut_annotations as
c_rgexp_identifier constant varchar2(50) := '[a-z][a-z0-9#_$]*';
c_annotation_block_pattern constant varchar2(200) := '(({COMMENT#.+}'||chr(10)||')+)( |'||chr(09)||')*(procedure|function)\s+(' ||
c_rgexp_identifier || ')';
c_annotation_pattern constant varchar2(50) := gc_annotation_qualifier || c_rgexp_identifier || '(\(.*?\)$)?';
c_annotation_pattern constant varchar2(50) := gc_annotation_qualifier || c_rgexp_identifier || '[ '||chr(9)||']*(\(.*?\)$)?';
Copy link
Member

Choose a reason for hiding this comment

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

Can you make the regexp work with trailing spaces?
'[ '||chr(9)||]((.?)[ '||chr(9)||']*$)?'

@coveralls
Copy link

Coverage Status

Coverage remained the same at 91.718% when pulling 4d9f9ae on Pazus:issue-357 into ea2573e on utPLSQL:develop.

1 similar comment
@coveralls
Copy link

coveralls commented Jul 2, 2017 •
edited
Loading

Coverage Status

Coverage remained the same at 91.718% when pulling 4d9f9ae on Pazus:issue-357 into ea2573e on utPLSQL:develop.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 91.718% when pulling 413e63f on Pazus:issue-357 into ea2573e on utPLSQL:develop.

1 similar comment
@coveralls
Copy link

coveralls commented Jul 3, 2017 •
edited
Loading

Coverage Status

Coverage remained the same at 91.718% when pulling 413e63f on Pazus:issue-357 into ea2573e on utPLSQL:develop.

@coveralls
Copy link

coveralls commented Jul 3, 2017 •
edited
Loading

Coverage Status

Coverage remained the same at 91.718% when pulling f1e22e4 on Pazus:issue-357 into ce2d8ad on utPLSQL:develop.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 91.718% when pulling f1e22e4 on Pazus:issue-357 into ce2d8ad on utPLSQL:develop.

@coveralls
Copy link

coveralls commented Jul 4, 2017 •
edited
Loading

Coverage Status

Coverage remained the same at 91.718% when pulling 2c4ffde on Pazus:issue-357 into f92772f on utPLSQL:develop.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 91.718% when pulling 2c4ffde on Pazus:issue-357 into f92772f on utPLSQL:develop.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 91.718% when pulling 76b2df3 on Pazus:issue-357 into 15a0922 on utPLSQL:develop.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 91.718% when pulling 76b2df3 on Pazus:issue-357 into 15a0922 on utPLSQL:develop.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 91.718% when pulling 76b2df3 on Pazus:issue-357 into 15a0922 on utPLSQL:develop.

@jgebal jgebal merged commit 380ff54 into utPLSQL:develop Jul 4, 2017
@jgebal jgebal removed the in progress label Jul 4, 2017
@Pazus Pazus deleted the issue-357 branch July 15, 2017 09:47
Pazus added a commit that referenced this pull request Jul 18, 2017
@Pazus
Merge branch 'release/v3.0.2'
64cd100 
### Documentation
  #399 Documentation now refers to [migration](https://github.com/utPLSQL/utPLSQL-v2-v3-migration) project
  #386 Documentation now refers to a valid object name: `ut_file_mapping`
  #362 Install and Uninstall scripts are now much more readable
  #361 Install guide now provides snippet on how to download latest release on Windows

### Installation
  #396 Added override user/password/tablespace for install_headless
  #384 Installation is now smooth even if profiler tables already exist

### Internal improvements
  #388 Improved reporting from RunTest
  #363 Fixed publishing of release documentation history

### Improvements and fixes
  #407 Fixed rare issue with `ORA-22813: operand value exceeds system limits`
  #403 Stack trace is now properly parsed on all machines
  #397 The `--%disabled` annotation on suite level is now reporting all tests as disabled
  #395 Coverage reporting is now properly filtering test packages on that use suitepath
  #390 Line of code for failed expectation is now also shown for unit tests owned by other users
  #380 Line no of failed test is now properly reported when using `ut.fail`
  #375 Annotation parameter list can now have spaces before/after brackets
  #373 Warnings in documentation reporter are now properly numbered
  #372 Documentation reporter is now providing a timing information for each test
  #370 by xUnit reporter now displays name of the package/procedure if suite/test has no description
  #369 Fixed errors with multi-byte characters in conversion from/to clob
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jgebal jgebal jgebal approved these changes

Assignees

@Pazus Pazus

Labels
bugfix
Projects
None yet
Milestone
v3.0.2
Development

Successfully merging this pull request may close these issues.
3 participants
@Pazus @coveralls @jgebal