New issue
Advanced search Search tips

Issue 773341 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Closed: Oct 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

chromeos-config-tools unit tests not running for cros_sdk build (host)

Project Member Reported by athilenius@chromium.org, Oct 10 2017

Issue description

crrev.com/c/704267 merged broken unit tests into cros/master. The unit tests pass for all boards, but not when run for the host (SDK). Why doesn't the CQ run host unit tests?
 
Cc: athilenius@chromium.org

Comment 2 by sjg@chromium.org, Oct 10 2017

Owner: sjg@chromium.org
Cc: shapiroc@chromium.org jclinton@chromium.org
Charles recently looked at a similar problem; he might have an idea.
It was this issue: crbug.com/769877. Hrm, the first comment there says you need "src_test()" or "platform_pkg_test()"; looks like you have it in http://cs/chromeos_public/src/third_party/chromiumos-overlay/chromeos-base/chromeos-config-tools/chromeos-config-tools-9999.ebuild?l=43&rcl=242a844379d825a59b86e5e5037745cb722c9cda .

The last build log is here: https://uberchromegw.corp.google.com/i/chromeos/builders/coral-paladin/builds/1408/steps/UnitTest/logs/stdio
Started chromeos-base/chromeos-config-tools-0.0.1-r639 (logged in /tmp/chromeos-config-tools-0.0.1-r639-aj3YXO)
...
Completed chromeos-base/chromeos-config-tools-0.0.1-r639 (in 0m35.9s)

That makes it look like it ran and passed.

Project Member

Comment 5 by bugdroid1@chromium.org, Oct 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/a59542ddfddbf5ff2934e8e40b4a6dc1ad0659c9

commit a59542ddfddbf5ff2934e8e40b4a6dc1ad0659c9
Author: Alec Thilenius <athilenius@chromium.org>
Date: Thu Oct 12 01:32:51 2017

chromeos-config: Fixing broken unit tests in cros/master

The commit crrev.com/c/704267 added a firmware node to caroline in the
test.dts file. The C++ library would open the firmware node, get the
property 'bcs-overlay' (which returned nullptr). That nullptr was casted
to a const char* and passed to a std::string constructor, resulting in a
segfault and dump.

This is a partial fix. It does not address the overarching issue: why
wasn't the unit test run for the host during CQ (see linked bug).

BUG= chromium:773341 
TEST=FEATURES=test sudo -E emerge chromeos-base/chromeos-config-tools

Change-Id: Ifcd3083de39fec73dcb1a287597a7fa66160b6c2
Reviewed-on: https://chromium-review.googlesource.com/713763
Commit-Ready: Alec Thilenius <athilenius@chromium.org>
Tested-by: Alec Thilenius <athilenius@chromium.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Jason Clinton <jclinton@chromium.org>

[modify] https://crrev.com/a59542ddfddbf5ff2934e8e40b4a6dc1ad0659c9/chromeos-config/libcros_config/cros_config.cc

Comment 6 by sjg@chromium.org, Oct 13 2017

Owner: athilenius@chromium.org
Status: Assigned (was: Untriaged)
Alec, can you do a tryjob with a CL that breaks the tests and see if it fails?

You have to do it in the chroot now unfortunately:

e.g.

cros tryjob -g 707174 coral-paladin coral-release

I have a failure, but I'm not sure it's related or that I started the tryjob correctly:

https://uberchromegw.corp.google.com/i/chromiumos.tryserver/builders/paladin/builds/3960

Comment 8 by sjg@chromium.org, Oct 13 2017

How did you start it? It seems to have my python install CL in there?
By running cros tryjob -g 707174 coral-paladin coral-release in the chroot. 707174 is the CL id right?

Comment 10 by sjg@chromium.org, Oct 13 2017

No, I mean run your own CL through the trybot, one that deliberately breaks a unit test. That CL is the one for installing the Python tools, so you don't want that.
Yep, it passed: https://uberchromegw.corp.google.com/i/chromiumos.tryserver/builders/paladin/builds/3962. Based on the CL crrev.com/c/719606 which has this in it:
```cpp
TEST(CrosConfigTest, ShouldFailInTryjob) {
  EXPECT_EQ("SHOULD FAIL IN TRYJOB", "");
}
```
Wasn't run by the TryJob.
Should I close this @sjg, or pass it off to you? If CQ wasn't running just our specific host test, then that's fine as it's deprecated. But if it isn't / wasn't running any host tests, that seems troubling.

Comment 13 by sjg@chromium.org, Oct 17 2017

You can just close it, as for the tests that matter, they are running
Status: WontFix (was: Assigned)

Sign in to add a comment