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

Fixing ticket 10174 #212

Merged
merged 3 commits into from Jan 21, 2013
Merged

Fixing ticket 10174 #212

merged 3 commits into from Jan 21, 2013

Conversation

hflynn
Copy link
Member

@hflynn hflynn commented Jan 16, 2013

No description provided.

@@ -112,7 +112,12 @@ example:
Failing tests
~~~~~~~~~~~~~

"test.with.fail" is the property which will
The ``test.with.fail`` ant property, which is ``true`` by default, can be set
Copy link
Member

Choose a reason for hiding this comment

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

It seems to default to false to me when I run unit tests. Inspection of lifecycle.xml reveals a few,

<property name="test.with.fail" value="false"/>

so it won't actually default to true in quite the cases that the reader may expect.

Copy link
Member

Choose a reason for hiding this comment

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

@mtbc, are you referring to all of the items which are proceeded with:

<!-- may be overriden by fail-on-error -->

? If so, the top-level setting to true in globals.xml should take precedence. (If anything we now need a dont-fail-on-error target to prevent that behavior.)

Can you give a specific example which defaults to false?

Copy link
Member

Choose a reason for hiding this comment

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

./build.py test-unit is much faster with -Dtest.with.fail=true and ends differently, and takes the same length of time and ends at the same place with -Dtest.with.fail=false.

@hflynn
Copy link
Member Author

hflynn commented Jan 17, 2013

Any verdict on this? Happy to change it if someone gives me alternative text to use.

@mtbc
Copy link
Member

mtbc commented Jan 17, 2013

If @joshmoore thinks the text is probably correct I'm fine with merging it now and our looking again at the situation another time. Even if it's misleading about the default for our builds, it may still be an overall improvement in pointing out that it's an ant property and what it's for: it's not like the consequences of wrongly thinking that it defaults to true are severe.

@joshmoore
Copy link
Member

@mtbc, my apologies, I found the reason. etc/local.properties.example (which sets the defaults) has the value specified:

  5 ############################################
  4 # Testing
  3 ############################################
  2 #
  1 # Prevents test failures from stopping the build
  0 test.with.fail=false

In order to not change functionality, let's use false then in the docs, and I'll add a clean-up of this situation to one of my open build branches.

@mtbc
Copy link
Member

mtbc commented Jan 18, 2013

Great, so it's presently false by default, only a small change needed to this PR.

I had noticed that file but it hadn't occurred to me that a file named *.example might actually be used for configuration, I thought it was, well, just an example of how to do it!

@hflynn
Copy link
Member Author

hflynn commented Jan 18, 2013

Better now?

@hflynn
Copy link
Member Author

hflynn commented Jan 18, 2013

3rd time lucky?

@mtbc
Copy link
Member

mtbc commented Jan 18, 2013

You probably don't need the example line with -Dtest.with.fail=false but, yes, either way I'm happy, good to merge. (-: (My 'including' was a poor choice of verb.)

@joshmoore
Copy link
Member

(No complaints here)

@hflynn
Copy link
Member Author

hflynn commented Jan 21, 2013

I'll rebase to develop as is then shall I?

@mtbc
Copy link
Member

mtbc commented Jan 21, 2013

Sure, go ahead.

jburel added a commit that referenced this pull request Jan 21, 2013
@jburel jburel merged commit fe3f70f into ome:dev_4_4 Jan 21, 2013
@gusferguson
Copy link

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

Successfully merging this pull request may close these issues.

None yet

5 participants