fix variable in catch statement

fix variable in catch statement

An oops after https://code.google.com/p/dart/source/detail?r=20051

Committed: https://code.google.com/p/dart/source/detail?r=20457

[kevmoo-old, 2013-03-22 13:35:42]

PTAL

[Siggi Cherem (dart-lang), 2013-03-22 21:08:17]

+efortuna who knows more about this configuration =)

https://codereview.chromium.org/12729026/diff/3001/pkg/unittest/lib/html_indi...
File pkg/unittest/lib/html_individual_config.dart (right):

https://codereview.chromium.org/12729026/diff/3001/pkg/unittest/lib/html_indi...
pkg/unittest/lib/html_individual_config.dart:30: var testGroupName =
parameter.split('=')[1];
would you mind instead pulling out the testGroupName declaration outside the
loop? I think that was what they wanted to show below (or both search +
testGroupName)

[Emily Fortuna, 2013-03-22 22:05:51]

https://codereview.chromium.org/12729026/diff/3001/pkg/unittest/lib/html_indi...
File pkg/unittest/lib/html_individual_config.dart (right):

https://codereview.chromium.org/12729026/diff/3001/pkg/unittest/lib/html_indi...
pkg/unittest/lib/html_individual_config.dart:30: var testGroupName =
parameter.split('=')[1];
On 2013/03/22 21:08:17, Siggi Cherem (dart-lang) wrote:
> would you mind instead pulling out the testGroupName declaration outside the
> loop? I think that was what they wanted to show below (or both search +
> testGroupName)

The previous refactor is strange. I think what you want is this:

void onStart() {
  var testGroupName = window.location.search;
  if (testGroupName != '') {
    try {
      for (var parameter in testGroupName.substring(1).split('&')) {
         if (parameter.startsWith('group=')) {
           testGroupName = parameter.split('=')[1];
         }
       }
       unittest.filterTests('^$testGroupName${unittest.groupSep}');
     } catch (e) {
       print('tried to match "$testGroupName"');
       print('NO_SUCH_TEST');
     }

OR, if you like having two separate variables, as Siggi was suggesting:
void onStart() {
  var search = window.location.search;
  var testGroupName;
  if (search != '') {
    try {
      for (var parameter in search.substring(1).split('&')) {
         if (parameter.startsWith('group=')) {
           testGroupName = parameter.split('=')[1];
         }
       }
       if (testGroupName != null) {
         unittest.filterTests('^$testGroupName${unittest.groupSep}');
       }
     } catch (e) {
       print('tried to match "$testGroupName"');
       print('NO_SUCH_TEST');
     }

basically my beef with the previous refactor is since it's in the for loop, it
gives the impression that we might call filterTests multiple times, but it
should only be called once.

but yes, the value in 35 should not be the full "search" string, but the sliced
testGroupName -- that error happens if we try to filter tests and come up with
nothing, so we want what we tried to filter with.

[kevmoo-old, 2013-03-22 22:19:48]

I was just trying to clean up an analyzer error. I don't ever use this config.

I'm happy to commit whatever the responsible party prefers.

[Emily Fortuna, 2013-03-22 22:29:35]

On 2013/03/22 22:19:48, kevmoo wrote:
> I was just trying to clean up an analyzer error. I don't ever use this config.
> 
> I'm happy to commit whatever the responsible party prefers.

And we appreciate you fixing analyzer errors!! Pete is out for the next week, so
its just us. Do as Siggi  says and add the variable up top and leave the error
as "$testGroupName" (that's my version 2) or go back to the way it was
originally written. It should still work. That change was not relevant to the
functionality that he added.

[kevmoo-old, 2013-03-23 22:12:02]

Now we're having fun.

So, the catch wasn't catching anything. I tried running the test with bad
groups, etc, but nothing happened. Unittest does not throw if a filter removes
all tests.

So...

1) Using a function to filter. Being clear this is a strict string match against
the beginning of the test definition
2) Throwing if there is more than one group
3) Throwing if a filter removes all tests.  This was the original goal, right?

Of course this brings about issues. What if the original test list was empty?
Should this config also throw in that case?

I still think this impl is an improvement.

PTAL

[blois, 2013-03-25 07:08:36]

On 2013/03/23 22:12:02, kevmoo wrote:
> Now we're having fun.
> 
> So, the catch wasn't catching anything. I tried running the test with bad
> groups, etc, but nothing happened. Unittest does not throw if a filter removes
> all tests.
> 
> So...
> 
> 1) Using a function to filter. Being clear this is a strict string match
against
> the beginning of the test definition
> 2) Throwing if there is more than one group
> 3) Throwing if a filter removes all tests.  This was the original goal, right?
> 
> Of course this brings about issues. What if the original test list was empty?
> Should this config also throw in that case?
> 
> I still think this impl is an improvement.
> 
> PTAL

I had been intending to fix this up, thanks Kevin! (and sorry for introducing
it!)

The original code was problematic because it was initializing testGroupName to
window.location.search, then overwriting it if the string followed a certain
pattern. This caused URLs which set the search string but did not have a valid
group= to throw an exception (and was probably why the try/catch was there-
because of bad regexp exceptions).

So, I refactored it to make testGroupName only be assigned if the search string
was parsed successfully, but missed the reference in the catch statement.

[Emily Fortuna, 2013-03-25 16:47:54]

lgtm. thanks, Kevin!

[blois, 2013-03-25 17:00:25]

lgtm

[kevmoo-old, 2013-03-25 17:17:44]

Committed patchset #3 manually as r20457 (presubmit successful).