Original intent of case sensitivity for choice type tokens

[mshriver]

I'm trying to determine what the original intent was for handling case sensitivity.

Per the docs (! Note under linked section) token matching is case insensitive. This appears to be the case for the tokens names themselves, which I have problem with.

https://testimony-qe.readthedocs.io/en/stable/#test-case-docstring-format

Concerning case sensitivity for token values, the picture isn't as clear. The unit test example configuration file uses a casesensitive field, which is only ever set to false for unit tests, or not at all.

https://github.com/SatelliteQE/testimony/blob/master/tests/config-full.yaml#L14

In use in robottelo, I would like to enforce capitalization on the token values for some of my 'choice' type tokens.

If I set casesensitive: true in configuration, or remove the field entirely, validation fails for all tokens - even if their capitalization matches the given options.

I'm trying to add a unit test to demonstrate this, but make tests fails for me locally. It passed in travis when 2.1 was released, so its likely something local. I can demonstrate this with robottelo's config if that's helpful.

[sthirugn]

hmm I dont remember using casesensitive. May be @elyezer would know?

[mirekdlugosz]

For the longest time, testimony only worked on tokens presence - it could tell if mandatory tokens are not present, and if token that is not on allowed list is present. Tokens were always considered to be case-insensitive.

Last year, we noticed that majority of tokens take only one of few allowed values, and testimony already has most of machinery to check that. This resulted in #141 and follow up PRs. Case-insensitivity of tokens extended to case-insensitivity of values in original implementation.

In #142 I wrote "I'm pretty sure everyone wants to have casesensitive: False", so I don't think we had any specific team in mind when implementing that. It just seemed to be reasonable addition - we don't care about case, but someone somewhere might have different opinion. It could also be added as demonstration - we taught testimony to validate token values, we have config file now, so let's show what is possible in this brand new world. Finally, it was just trivial to add - patch for that was literally 3 lines of code.

If I set casesensitive: true in configuration, or remove the field entirely, validation fails for all tokens - even if their capitalization matches the given options.

That looks like a bug. Following patch seems to fix it:

diff --git testimony/config.py testimony/config.py
index 7de11e6..38aef77 100644
--- testimony/config.py
+++ testimony/config.py
@@ -75,8 +75,8 @@ class TokenConfig(object):
         if self.token_type == 'choice':
             assert 'choices' in config
             assert isinstance(config['choices'], list)
-            casesensitive = config.get('casesensitive', True)
-            self.choices = [i if casesensitive else i.lower()
+            self.casesensitive = config.get('casesensitive', True)
+            self.choices = [i if self.casesensitive else i.lower()
                             for i in config['choices']]
 
     def update(self, new_values):
@@ -87,5 +87,7 @@ class TokenConfig(object):
     def validate(self, what):
         """Ensure that 'what' meets value validation criteria."""
         if self.token_type == 'choice':
-            return what.lower() in self.choices
+            if not self.casesensitive:
+                what = what.lower()
+            return what in self.choices
         return True  # assume valid for unknown types

[elyezer]

@mirekdlugosz got it very well covered already. I had a hard time trying to remember since it has been a long time that I don't come back to this code base.

@mirekdlugosz are you sending a PR or want me to fix it?

[mirekdlugosz]

@elyezer feel free to take it over.
I thought it would be nice to add some test case(s) for this, and get confirmation from @mshriver this patch indeed solves the problem, and I don't have much time for all this right now.

[mshriver]

I'm also happy to pull that patch into a PR and include a unit test for it - after all its behavior that I want to support. I'm seeing 3 instance of line number differences in the sample_output for unit tests on master branch commit though?

setup@localhost:~/repos/testimony (broken-capitalization%) % make test
314c314
< DecoratorsTestCase::test_one_decorator:251
---
> DecoratorsTestCase::test_one_decorator:252
333c333
< DecoratorsTestCase::test_multiple_decorators:256
---
> DecoratorsTestCase::test_multiple_decorators:258
371c371
< MergeDecoratorsTestCase::test_decorator:275
---
> MergeDecoratorsTestCase::test_decorator:276

Travis job for 2.1 release didn't fail on this, but I'm not sure why/how I'm hitting it on an unmodified master branch. I've gotta sort that before I can reliably submit the PR with unit tests. Curious if any of you see this on master branch running make test? I've created a fresh venv for this, following how the setup and testing is done in CI.

[elyezer]

I tried to run make test locally and I am not facing that issue, I just pulled from master and I am using the commit 094e578.

$ make test
$ echo $?        
0

[mshriver]

Yep, on the same commit with the failure in python 3.8.6 - but I switched to py3.6 and make test passes, so that piece is separate. I'll include Mirek's commit and a unit test in a new PR.