Re: [Freeipa-devel] [PATCHES] 0094-0099 Make Fuzzy objects available for the whole ipatests module
On 09/16/2013 02:36 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
On 09/13/2013 07:04 PM, Rob Crittenden wrote:
[...]
Replacing it would either a) replicate its functionality almost
completely or b) spread duplicate regex code all over the place.
I'd go for b; spreading this code:
import re
some_regex = re.compile('some.*regex$')
...
assert some_regex.search(x)
instead of:
from wherever import Fuzzy
warm_fuzzy = Fuzzy('some.*regex$')
...
assert x == warm_fuzzy
all over the place is fine in my book. And you can even, say, add custom
flags to the regex without complicating shared code.
Right, and it's all duplicate. Just look in his patch how many times
fuzzy.digits is used. What's going to happen is someone is going to come
along later and say "Geez, we have a ton of some_regex =
re.compile('\d+'), I should make a macro thinger out of this" and we're
back where we started.
The first two lines only need to be there once, in the other files you
can just import some_regex, the same way you can import the Fuzzy object.
The rest of Fuzzy functionality consists of strict type checking (which
isn't really necessary in integration tests), and the ability to call
arbitrary callables (which is just the scope creep I was talking about).
By the way, in current tests these features are hardly ever used in
combination.
Here we agree. I'd prefer to keep Fuzzy limited to just simple regex and
woudln't object to delegating the other enforcement to something else.
The type checking is actually a big part of Fuzzy functionality, since
in the API we want all non-binary strings to be unicode and not str.
That isn't to say that Fuzzy isn't being abused, but that is also the
responsibility of the reviewers to be strict about.
Then perhaps I'm too strict, but I say that using it outside of the
declarative tests is abuse.
Especially if it takes six patches with hundreds of changed lines to
repurpose Fuzzy for integration tests (but that's not part of "-1 to the
idea").
That is hardly fair. The bulk of the patches just change imports.
The patches make Fuzzy enforce basestring type by default, instead of
unicode. But in the IPA API we normally want only unicode, so almost all
*usages* of Fuzzy are then changed to enforce unicode.
That is bad because IMO Fuzzy is specific to the declarative API tests
and should have defaults made for them.
So to summarize, I think:
- Fuzzy should remain as a regex should remain as a regex shortcut
- The non-regex features can be moved elsewhere
- I don't really have a handle on how he intended to use this for
integration testing, so I don't have an opinion here. But I'd expect
that most integration tests depend more on return values than comparing
against "known good".
We're getting closer to an agreement :)
- Fuzzy should remain as a regex should remain as a regex shortcut *for
our declarative tests which need type-checking*
- The non-regex *and non-typechecking* features can be moved elsewhere
(they actually are: assert_deepequal allows plain callables, it's just a
matter of using that in the tests and then removing the functionality
from Fuzzy)
- In integration testing, if we do need to check the output of commands,
we don't really care about the bytes/unicode distinction, so the re
module is enough.
--
Petr³
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 0094-0099 Make Fuzzy objects available for the whole ipatests module
Petr Viktorin wrote:
On 09/13/2013 07:04 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
On 09/13/2013 04:12 PM, Tomas Babej wrote:
Hi,
The following patches move and extend the functionality of Fuzzy
objects. This is necessary to use them from within integration tests.
-1 to the idea.
I'm not a fan of the Fuzzy objects; they basically exist so that you can
use regex matching in the Declarative tests.
As you've probably noticed Fuzzy is quite specific to the framework and
its test suite -- see the strict bytes/unicode separation and the amount
of changes it takes to tear them out.
I'm also not a fan of having a generic "Match anything using some rules"
object where everybody adds behavior specific to their use case. Each
addition increases the complexity and number of corner cases, and the
complete intended functionality can never be achieved.
Using regular expressions directly should actually be easier and less
error-prone in most cases.
If you disagree, I'd like to see your use case.
I'm not sure what the objection is, Fuzzy is just an abstraction. It
lets one do in-line regex which is the reason it was introduced
(initially for uid and gid because they are non-deterministic).
Yes. "In-line" is the key here. It lets you do regex matching via the ==
operator, which is what Declarative tests need, but elsewhere the
abstraction is completely unnecessary.
Replacing it would either a) replicate its functionality almost
completely or b) spread duplicate regex code all over the place.
I'd go for b; spreading this code:
import re
some_regex = re.compile('some.*regex$')
...
assert some_regex.search(x)
instead of:
from wherever import Fuzzy
warm_fuzzy = Fuzzy('some.*regex$')
...
assert x == warm_fuzzy
all over the place is fine in my book. And you can even, say, add custom
flags to the regex without complicating shared code.
Right, and it's all duplicate. Just look in his patch how many times
fuzzy.digits is used. What's going to happen is someone is going to come
along later and say "Geez, we have a ton of some_regex =
re.compile('\d+'), I should make a macro thinger out of this" and we're
back where we started.
The rest of Fuzzy functionality consists of strict type checking (which
isn't really necessary in integration tests), and the ability to call
arbitrary callables (which is just the scope creep I was talking about).
By the way, in current tests these features are hardly ever used in
combination.
Here we agree. I'd prefer to keep Fuzzy limited to just simple regex and
woudln't object to delegating the other enforcement to something else.
Even if this extra functionality is necessary, it's orthogonal to regex
matching and can be more cleanly done with several separate asserts.
Unless you need a single declarative object to compare against, of course.
Yup.
That isn't to say that Fuzzy isn't being abused, but that is also the
responsibility of the reviewers to be strict about.
Then perhaps I'm too strict, but I say that using it outside of the
declarative tests is abuse.
Especially if it takes six patches with hundreds of changed lines to
repurpose Fuzzy for integration tests (but that's not part of "-1 to the
idea").
That is hardly fair. The bulk of the patches just change imports.
So to summarize, I think:
- Fuzzy should remain as a regex should remain as a regex shortcut
- The non-regex features can be moved elsewhere
- I don't really have a handle on how he intended to use this for
integration testing, so I don't have an opinion here. But I'd expect
that most integration tests depend more on return values than comparing
against "known good".
rob
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 0094-0099 Make Fuzzy objects available for the whole ipatests module
On 09/13/2013 07:04 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
On 09/13/2013 04:12 PM, Tomas Babej wrote:
Hi,
The following patches move and extend the functionality of Fuzzy
objects. This is necessary to use them from within integration tests.
-1 to the idea.
I'm not a fan of the Fuzzy objects; they basically exist so that you can
use regex matching in the Declarative tests.
As you've probably noticed Fuzzy is quite specific to the framework and
its test suite -- see the strict bytes/unicode separation and the amount
of changes it takes to tear them out.
I'm also not a fan of having a generic "Match anything using some rules"
object where everybody adds behavior specific to their use case. Each
addition increases the complexity and number of corner cases, and the
complete intended functionality can never be achieved.
Using regular expressions directly should actually be easier and less
error-prone in most cases.
If you disagree, I'd like to see your use case.
I'm not sure what the objection is, Fuzzy is just an abstraction. It
lets one do in-line regex which is the reason it was introduced
(initially for uid and gid because they are non-deterministic).
Yes. "In-line" is the key here. It lets you do regex matching via the ==
operator, which is what Declarative tests need, but elsewhere the
abstraction is completely unnecessary.
Replacing it would either a) replicate its functionality almost
completely or b) spread duplicate regex code all over the place.
I'd go for b; spreading this code:
import re
some_regex = re.compile('some.*regex$')
...
assert some_regex.search(x)
instead of:
from wherever import Fuzzy
warm_fuzzy = Fuzzy('some.*regex$')
...
assert x == warm_fuzzy
all over the place is fine in my book. And you can even, say, add custom
flags to the regex without complicating shared code.
The rest of Fuzzy functionality consists of strict type checking (which
isn't really necessary in integration tests), and the ability to call
arbitrary callables (which is just the scope creep I was talking about).
By the way, in current tests these features are hardly ever used in
combination.
Even if this extra functionality is necessary, it's orthogonal to regex
matching and can be more cleanly done with several separate asserts.
Unless you need a single declarative object to compare against, of course.
That isn't to say that Fuzzy isn't being abused, but that is also the
responsibility of the reviewers to be strict about.
Then perhaps I'm too strict, but I say that using it outside of the
declarative tests is abuse.
Especially if it takes six patches with hundreds of changed lines to
repurpose Fuzzy for integration tests (but that's not part of "-1 to the
idea").
--
Petr³
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 0094-0099 Make Fuzzy objects available for the whole ipatests module
Petr Viktorin wrote: On 09/13/2013 04:12 PM, Tomas Babej wrote: Hi, The following patches move and extend the functionality of Fuzzy objects. This is necessary to use them from within integration tests. -1 to the idea. I'm not a fan of the Fuzzy objects; they basically exist so that you can use regex matching in the Declarative tests. As you've probably noticed Fuzzy is quite specific to the framework and its test suite -- see the strict bytes/unicode separation and the amount of changes it takes to tear them out. I'm also not a fan of having a generic "Match anything using some rules" object where everybody adds behavior specific to their use case. Each addition increases the complexity and number of corner cases, and the complete intended functionality can never be achieved. Using regular expressions directly should actually be easier and less error-prone in most cases. If you disagree, I'd like to see your use case. I'm not sure what the objection is, Fuzzy is just an abstraction. It lets one do in-line regex which is the reason it was introduced (initially for uid and gid because they are non-deterministic). Replacing it would either a) replicate its functionality almost completely or b) spread duplicate regex code all over the place. That isn't to say that Fuzzy isn't being abused, but that is also the responsibility of the reviewers to be strict about. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 0094-0099 Make Fuzzy objects available for the whole ipatests module
On 09/13/2013 04:12 PM, Tomas Babej wrote: Hi, The following patches move and extend the functionality of Fuzzy objects. This is necessary to use them from within integration tests. -1 to the idea. I'm not a fan of the Fuzzy objects; they basically exist so that you can use regex matching in the Declarative tests. As you've probably noticed Fuzzy is quite specific to the framework and its test suite -- see the strict bytes/unicode separation and the amount of changes it takes to tear them out. I'm also not a fan of having a generic "Match anything using some rules" object where everybody adds behavior specific to their use case. Each addition increases the complexity and number of corner cases, and the complete intended functionality can never be achieved. Using regular expressions directly should actually be easier and less error-prone in most cases. If you disagree, I'd like to see your use case. -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
