Mark Côté [:mcote] Reporter
	Description • 9 years ago

Upstream's Selenium-based test suite does not run properly against BMO.  We should fix it in our fork and getting it running, ideally on Taskcluster (see blocker).

	David Lawrence [:dkl] Assignee
	Comment 1 • 9 years ago

- test_votes.t and test_edit_bug_properties.t waiting on upstream fixes for bug (checked into bmo/development)
- dklawren/webtools-bmo-qa docker image still pulls from mozilla/webtools-bmo-qa github repo for tests and will be converted to
  use the local qa/ directory once these scripts are committed.
- test_email_preferences.t still failing and investigating the BMO'ism that may be causing this. Email preferences debugging is not for the timid.

Command to start test suites:

Note should work for boot2docker once it is configured properly on OSX. On Linux, docker just needs to be installed.

For sanity tests only:
$ docker run -it dklawren/webtools-bmo-bugzilla /runtests.sh

For Selenium tests:
$ docker run -it -p 5900:5900 -e TEST_SUITE=selenium -e GITHUB_BASE_BRANCH=development dklawren/webtools-bmo-bugzilla /runtests.sh

For WebService tests:
$ docker run -it -e TEST_SUITE=webservices -e GITHUB_BASE_BRANCH=development dklawren/webtools-bmo-bugzilla /runtests.sh

The '-p 5900:5900' allows for a local VNC client such as tigervnc (Linux) and Chicken of the VNC (OSX) to follow along the Selenium tests as they happen.

Recent test runs of my own can be seen here:
https://treeherder.mozilla.org/#/jobs?repo=bmo-development

Let me know if I can provide more detailed information on what to look at. The test script that does the grunt work is docker/runtests.sh which is embedded in the docker image itself but could be moved to qa/ at some point to make the stock image more lean and focused only on running the Bugzilla system.

Thank you for your consideration
dkl

	Dylan Hardison [:dylan] (he/him)
	Comment 2 • 9 years ago

Comment on attachment 8621002 [details] [diff] [review]
1144485_1.patch

Review of attachment 8621002 [details] [diff] [review]:
-----------------------------------------------------------------

Overall I like this patch. The tests could be modernized (but that's not something we need to do now)
and there's the possibly useless code in qa/extensions. I would eventually like the docker tests to the use the current checkout, as we discussed with ed morley.

I will take a very close look at the run_tests script and run it on a "fresh" docker host machine, but I an r- at this point is not likely.

	:glob ✱
	Comment 3 • 9 years ago

Comment on attachment 8621002 [details] [diff] [review]
1144485_1.patch

Review of attachment 8621002 [details] [diff] [review]:
-----------------------------------------------------------------

this looks good to me, aside from generate_test_data.pl needing some code style tweaks.

::: qa/config/generate_test_data.pl
@@ +1,1 @@
> +#!/usr/bin/perl -w

i see a lot of overlap between this code and docker/generate_bmo_data.pl
would it be feasible to combine the two scripts?  (building a fuller bmo environment that contains our hard coded groups and users is likely to ease development of bmo-specific test scripts).

there's some code style issues here, such as using "for" instead of "foreach", a mode line, extra spaces inside parentheses "if ( condition ) {", etc.  these need to be cleaned up to match our existing code.

@@ +188,5 @@
> +##########################################################################
> +# Bug statuses
> +##########################################################################
> +
> +# We need to add in the upstream statuses in addition to the BMO ones.

why?

@@ +759,5 @@
> +print "creating attachments...\n";
> +# We use the contents of this script as the attachment.
> +open(my $attachment_fh, '<', __FILE__) or die __FILE__ . ": $!";
> +my $attachment_contents;
> +{ local $/; $attachment_contents = <$attachment_fh>; }

split into multiple lines

::: qa/t/test_bmo_autolinkification.t
@@ +18,5 @@
> +log_in($sel, $config, 'unprivileged');
> +file_bug_in_product($sel, 'TestProduct');
> +my $bug_summary = "linkification test bug";
> +$sel->type_ok("short_desc", $bug_summary);
> +# $sel->type_ok("comment", "crash report: bp-63f096f7-253b-4ee2-ae3d-8bb782090824\ncve: CVE-2010-2884\nsvn: r12345");

this commented out line should be deleted

@@ +48,5 @@
> +$sel->title_like(qr/\d+ \S $bug_summary/, "svn revision added");
> +$sel->click_ok("link=bug $bug_id");
> +$sel->wait_for_page_to_load_ok(WAIT_TIME);
> +$sel->attribute_is('link=r12345@href', 'https://viewvc.svn.mozilla.org/vc?view=rev&revision=12345');
> +

missing tests for git.mozilla.org and hg.mozilla.org linkification

	Dylan Hardison [:dylan] (he/him)
	Comment 4 • 9 years ago

Comment on attachment 8621002 [details] [diff] [review]
1144485_1.patch

Review of attachment 8621002 [details] [diff] [review]:
-----------------------------------------------------------------

r=dylan

with some notes.

::: qa/config/generate_test_data.pl
@@ +28,5 @@
> +
> +    $conf_path = $config->{bugzilla_path};
> +}
> +
> +use lib $conf_path;

It surprised me that this works, with the lexical being declared outside the BEGIN-time block, and then being passed to lib. I guess that is how that works. :-)

I would choose to de-sugar the "use lib" into:
require lib;
lib->import($conf_path); from inside the BEGIN block, but there's no benefit from that. other than being less confusing.

@@ +756,5 @@
> +    $sth_add_mapping->execute( Bugzilla->user->id, $group->id, 0, GRANT_DIRECT );
> +};
> +
> +print "creating attachments...\n";
> +# We use the contents of this script as the attachment.

note: you could seek the DATA file handle and read in the whole script that way.

@@ +806,5 @@
> +# Install the QA extension #
> +############################
> +
> +print "copying the QA extension...\n";
> +my $output = `cp -R ../extensions/QA $conf_path/extensions/.`;

I would use File::Copy here, as it is more reliable/portable/etc than shelling out to cp.

@@ +812,5 @@
> +
> +my $cwd = cwd();
> +chdir($conf_path);
> +$output = `perl contrib/fixperms.pl`;
> +print $output if $output;

system("perl", "contrib/fixperms.pl"); 

There's no reason to use backticks for this, as we don't care about the output.

	David Lawrence [:dkl] Assignee
	Comment 5 • 9 years ago

Filing additional bugs to deal with a few of the remaining suggested changes

To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   28f425d..a6238e0  master -> master

dkl