New issue
Advanced search Search tips

Issue 880661 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 11
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Feature



Sign in to add a comment

Make Tast validate that test function names and filenames match

Project Member Reported by derat@chromium.org, Sep 5

Issue description

I plan to make testing.AddTest verify that tests' function names match their filenames. For example, a test named "MyTest" should be registered from a file named my_test.go.

There's still some ambiguity here due to acronyms/initialisms, i.e. a test function named PlayMP3 could be registered by a file named play_mp3.go, play_m_p3.go, or other similar variants. Similarly, acronyms/initialisms are supposed to use a constant case in Go, but we don't have any way of knowing that a file named check_ssid.go should register a test function named CheckSSID rather than one named CheckSsid.

I think it'd make sense to do as much as we can to catch incorrect names, though.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Sep 6

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast-tests/+/504217bbac9a6f14f953177ec2258f36790371da

commit 504217bbac9a6f14f953177ec2258f36790371da
Author: Daniel Erat <derat@chromium.org>
Date: Thu Sep 06 04:04:34 2018

tast-tests: Make "Crosvm" naming consistent.

Rename instances of "CrosVM" to "Crosvm" to follow the
existing convention of using underscores to separate words
in filenames and capitalization to separate words in
function (and test) names.

The naming of the vm.StartCrosVM test conflicts with a
change I'm working on to verify that filenames and test
names are aligned, since it's registered by a file named
start_crosvm.go (rather than start_cros_vm.go).

The vm.CrosVM struct currently lives in cros_vm.go, so
another option would be to rename the test file to
start_cros_vm.go. As I understand it, the actual hypervisor
that's being tested is named "crosvm", though, so it feels a
bit less surprising to me to make the filenames follow that
and change the capitalization in code to match.

(I also see that the name was written as "Crosvm" in the
subject of the initial announcement at
http://g/chromeos-rust/XNVRa6oy3-0/pCVc5_qIBAAJ.)

BUG= chromium:880661 
TEST=updated "tast run" doesn't complain about test name

Change-Id: I49c68dec2cc0a5edbf0743a1e0781d341c6a509d
Reviewed-on: https://chromium-review.googlesource.com/1207212
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Stephen Barber <smbarber@chromium.org>
Reviewed-by: Dylan Reid <dgreid@chromium.org>

[modify] https://crrev.com/504217bbac9a6f14f953177ec2258f36790371da/src/chromiumos/tast/local/bundles/cros/vm/start_crosvm.go
[rename] https://crrev.com/504217bbac9a6f14f953177ec2258f36790371da/src/chromiumos/tast/local/vm/crosvm.go

Project Member

Comment 2 by bugdroid1@chromium.org, Sep 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast/+/ed18ab60b03ce0c314ba121d5f4309743a377bf8

commit ed18ab60b03ce0c314ba121d5f4309743a377bf8
Author: Daniel Erat <derat@chromium.org>
Date: Sat Sep 08 01:36:52 2018

tast: Add testing.SetGlobalRegistryForTesting.

Remove the existing testing.ClearForTesting function and add
a new testing.SetGlobalRegistryForTesting function that unit
tests can call to temporarily install their own
testing.Registry. A followup change will use this to let
tests set a specially-configured registry.

BUG= chromium:880661 
TEST=unit tests still pass

Change-Id: I94b1a95c32019648fb4238bb9a1498b0100b5559
Reviewed-on: https://chromium-review.googlesource.com/1206070
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Shuhei Takahashi <nya@chromium.org>

[modify] https://crrev.com/ed18ab60b03ce0c314ba121d5f4309743a377bf8/src/chromiumos/tast/bundle/local_test.go
[modify] https://crrev.com/ed18ab60b03ce0c314ba121d5f4309743a377bf8/src/chromiumos/tast/testing/global.go
[modify] https://crrev.com/ed18ab60b03ce0c314ba121d5f4309743a377bf8/src/chromiumos/tast/bundle/remote_test.go
[modify] https://crrev.com/ed18ab60b03ce0c314ba121d5f4309743a377bf8/src/chromiumos/tast/bundle/bundle_test.go
[modify] https://crrev.com/ed18ab60b03ce0c314ba121d5f4309743a377bf8/src/chromiumos/tast/bundle/args_test.go

Project Member

Comment 3 by bugdroid1@chromium.org, Sep 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast/+/0aa9d36e5363e38a049dd69731e57387b0730893

commit 0aa9d36e5363e38a049dd69731e57387b0730893
Author: Daniel Erat <derat@chromium.org>
Date: Sat Sep 08 17:07:15 2018

tast: Validate test names against filenames.

Make testing.Test verify that test function names (from
which test names are derived) match the names of the files
that register the tests.

For example, a test function "MyNewTest" should be
registered by a file named "my_new_test.go". Acronyms are
permitted; a test named "PlayMP3" can be registered by
"play_mp3.go".

If a test is non-conforming, an error will be produced when
the bundle is executed, e.g.

  Failed building or pushing tests: failed to get data file
  list: Process exited with status 5: exit status 3
  (error(s) in registered tests: .../start_crosvm.go:19:
  word "CrosVM" in "StartCrosVM" has uppercase "V" after
  lowercase letter)

BUG= chromium:880661 
TEST=added unit tests; also verified that local and remote
     "cros" test bundles run without errors (after renaming
     vm.StartCrosVM to vm.StartCrosvm)
CQ-DEPEND=I49c68dec2cc0a5edbf0743a1e0781d341c6a509d

Change-Id: Ib4ac82dbb2fce1216e802329cd0c75b11bda33ed
Reviewed-on: https://chromium-review.googlesource.com/1208236
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/0aa9d36e5363e38a049dd69731e57387b0730893/src/chromiumos/tast/bundle/local_test.go
[modify] https://crrev.com/0aa9d36e5363e38a049dd69731e57387b0730893/src/chromiumos/tast/runner/runner_test.go
[modify] https://crrev.com/0aa9d36e5363e38a049dd69731e57387b0730893/src/chromiumos/tast/testing/test.go
[modify] https://crrev.com/0aa9d36e5363e38a049dd69731e57387b0730893/src/chromiumos/tast/testing/registry.go
[modify] https://crrev.com/0aa9d36e5363e38a049dd69731e57387b0730893/src/chromiumos/tast/testing/registry_test.go
[modify] https://crrev.com/0aa9d36e5363e38a049dd69731e57387b0730893/src/chromiumos/tast/bundle/remote_test.go
[modify] https://crrev.com/0aa9d36e5363e38a049dd69731e57387b0730893/src/chromiumos/tast/testing/test_test.go
[modify] https://crrev.com/0aa9d36e5363e38a049dd69731e57387b0730893/src/chromiumos/tast/bundle/args_test.go
[modify] https://crrev.com/0aa9d36e5363e38a049dd69731e57387b0730893/src/chromiumos/tast/bundle/bundle_test.go

Project Member

Comment 4 by bugdroid1@chromium.org, Sep 11

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast-tests/+/34c450cd5e9402fd5f801c6fc94e5643668542de

commit 34c450cd5e9402fd5f801c6fc94e5643668542de
Author: Daniel Erat <derat@chromium.org>
Date: Tue Sep 11 00:17:42 2018

tast-tests: Add test registration unit tests to bundles.

Add trivial unit tests to the local and remote "cros" test
bundle packages that verify that
testing.RegistrationErrors() is empty.

This detects issues in testing.Test{} structs (and soon,
test filename issues) that would otherwise only be caught
later when the test bundle is executed.

BUG= chromium:880661 
TEST=manual: added a negative timeout to a test and verified
     that unit tests fail

Change-Id: I82e904dc190f63f20f1be3566537d3005803e459
Reviewed-on: https://chromium-review.googlesource.com/1213780
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Shuhei Takahashi <nya@chromium.org>

[add] https://crrev.com/34c450cd5e9402fd5f801c6fc94e5643668542de/src/chromiumos/tast/local/bundles/cros/main_test.go
[add] https://crrev.com/34c450cd5e9402fd5f801c6fc94e5643668542de/src/chromiumos/tast/remote/bundles/cros/main_test.go

Status: Fixed (was: Started)
Project Member

Comment 6 by bugdroid1@chromium.org, Sep 14

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast/+/0db61a4998af8a1d39171bf7f6911a42b1536c39

commit 0db61a4998af8a1d39171bf7f6911a42b1536c39
Author: Daniel Erat <derat@chromium.org>
Date: Fri Sep 14 19:08:23 2018

tast: Relax test naming rules to permit trailing acronyms.

Relax testing.Test's validation code to permit strings like
"WebRTC" and "CrosVM" to appear as "webrtc" and "crosvm" in
test filenames rather than needing to be "web_rtc" and
"cros_vm".

BUG= chromium:880661 
TEST=updated unit test

Change-Id: I34a7540b39501fd4c1180fe9fbd64d1e5d3a46f7
Reviewed-on: https://chromium-review.googlesource.com/1222293
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Reviewed-by: Shuhei Takahashi <nya@chromium.org>

[modify] https://crrev.com/0db61a4998af8a1d39171bf7f6911a42b1536c39/src/chromiumos/tast/testing/test.go
[modify] https://crrev.com/0db61a4998af8a1d39171bf7f6911a42b1536c39/src/chromiumos/tast/testing/test_test.go

Sign in to add a comment