New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 905426 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 13
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocked on:
issue 835690



Sign in to add a comment

typ expectation parser should raise error upon detecting expectation line whose multiple exclusive specifiers

Project Member Reported by nedngu...@google.com, Nov 14

Issue description

Example test case:

# tags: [ toyota honda bmw ]
# tags: [ foo bar ]

 crbug.com/1234  [ toyota bmw foo ] drive/to/work [ Failure ] 


Parsing that expectation above should throw error since "drive/to/work" tests has "toyota" & "bmw" specifiers which are meant to be mutually exclusive (as they belong in a same tags group)
 
Blocking: 835690
Cc: -rmhasan@google.com dpranke@chromium.org
Owner: rmhasan@google.com
Talked to Dirk, this would be a good first bug for Rakib to get into core implementation of the expectation :-)
There is also no verification of whether the tag sets are mutually exclusive.
Can we address the issue I mentioned in my comment above in this issue.
#4: yup, that would be great as well. Can you also work on addressing it?
Yes I can work on addressing that problem as well. 
Cc: nedngu...@google.com
Ned recently added a similar lint rule to web_tests: https://crrev.com/c/1336439
Project Member

Comment 9 by bugdroid1@chromium.org, Dec 6

The following revision refers to this bug:
  https://chromium.googlesource.com/catapult/+/41fdb78fbbd83b6da4ef026d64ae1e023787a50d

commit 41fdb78fbbd83b6da4ef026d64ae1e023787a50d
Author: Rakib M. Hasan <rmhasan@google.com>
Date: Thu Dec 06 01:49:14 2018

Adds new verification to expectation_parser.py

Currently typ accepts tags that are a part of multiple tag sets.
This change will add a new verification that will make sure each
tag set is mutually exclusive. If this rule is violated, then a
ParseError exception will be thrown and will contain an appropriate
message. I also added a new unittest,
testParseExpectationMutuallyExclusiveTestSets in
expectations_parser_test.py for this change

R=dpranke@google.com, nednguyen@google.com

Bug:  chromium:905426 
Change-Id: Ieb2ed64c6a3abd8b7adbfa5144c15e43ffed40ca
Reviewed-on: https://chromium-review.googlesource.com/c/1359875
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Ned Nguyen <nednguyen@google.com>
Commit-Queue: Rakib Hasan <rmhasan@google.com>

[modify] https://crrev.com/41fdb78fbbd83b6da4ef026d64ae1e023787a50d/third_party/typ/typ/tests/expectations_parser_test.py
[modify] https://crrev.com/41fdb78fbbd83b6da4ef026d64ae1e023787a50d/third_party/typ/typ/expectations_parser.py

Project Member

Comment 10 by bugdroid1@chromium.org, Dec 6

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7c2c6d0b62ba27d74d76d889cac156032508ada2

commit 7c2c6d0b62ba27d74d76d889cac156032508ada2
Author: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Date: Thu Dec 06 03:30:29 2018

Roll src/third_party/catapult a8f4725aa003..41fdb78fbbd8 (1 commits)

https://chromium.googlesource.com/catapult.git/+log/a8f4725aa003..41fdb78fbbd8


git log a8f4725aa003..41fdb78fbbd8 --date=short --no-merges --format='%ad %ae %s'
2018-12-06 rmhasan@google.com Adds new verification to expectation_parser.py


Created with:
  gclient setdep -r src/third_party/catapult@41fdb78fbbd8

The AutoRoll server is located here: https://autoroll.skia.org/r/catapult-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.

CQ_INCLUDE_TRYBOTS=luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel

BUG= chromium:905426 
TBR=sullivan@chromium.org

Change-Id: Iadc0d06b2a4c70581e69076309c1b0be42ad0aec
Reviewed-on: https://chromium-review.googlesource.com/c/1364178
Reviewed-by: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Commit-Queue: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#614261}
[modify] https://crrev.com/7c2c6d0b62ba27d74d76d889cac156032508ada2/DEPS

Project Member

Comment 11 by bugdroid1@chromium.org, Dec 6

The following revision refers to this bug:
  https://chromium.googlesource.com/catapult/+/62ba44ace70cac953c1627cf5d6c7d92f51d47cf

commit 62ba44ace70cac953c1627cf5d6c7d92f51d47cf
Author: Rakib M. Hasan <rmhasan@google.com>
Date: Thu Dec 06 23:43:02 2018

Adds another verification to the Test expectation parser

This change will add a new check which will make sure
that any tags within a tag group are from different
tag sets. If they are from the tag set then a ParseError
exception will be thrown with an appropriate message. I
also had to make a change to unit test
testParseExpectationLineMultipleTags because the test
expectation had tags from the same tag set.

R=nednguyen@google.com

Bug:  chromium:905426 
Change-Id: I21ac98e7ea86d582b00c5a96d8b9efa8d5bf7008
Reviewed-on: https://chromium-review.googlesource.com/c/1362533
Commit-Queue: Rakib Hasan <rmhasan@google.com>
Reviewed-by: Ned Nguyen <nednguyen@google.com>

[modify] https://crrev.com/62ba44ace70cac953c1627cf5d6c7d92f51d47cf/third_party/typ/typ/tests/expectations_parser_test.py
[modify] https://crrev.com/62ba44ace70cac953c1627cf5d6c7d92f51d47cf/third_party/typ/typ/expectations_parser.py

Project Member

Comment 12 by bugdroid1@chromium.org, Dec 7

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a61e410e3beb88a04880f973dbde7c0099dbdff6

commit a61e410e3beb88a04880f973dbde7c0099dbdff6
Author: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Date: Fri Dec 07 06:29:16 2018

Roll src/third_party/catapult 5d7bcad36ff4..62ba44ace70c (1 commits)

https://chromium.googlesource.com/catapult.git/+log/5d7bcad36ff4..62ba44ace70c


git log 5d7bcad36ff4..62ba44ace70c --date=short --no-merges --format='%ad %ae %s'
2018-12-06 rmhasan@google.com Adds another verification to the Test expectation parser


Created with:
  gclient setdep -r src/third_party/catapult@62ba44ace70c

The AutoRoll server is located here: https://autoroll.skia.org/r/catapult-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.

CQ_INCLUDE_TRYBOTS=luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel

BUG= chromium:905426 
TBR=sullivan@chromium.org

Change-Id: Ibda0e80cd77c7ecd1434701f064e2f99db1b8307
Reviewed-on: https://chromium-review.googlesource.com/c/1367099
Reviewed-by: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Commit-Queue: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#614625}
[modify] https://crrev.com/a61e410e3beb88a04880f973dbde7c0099dbdff6/DEPS

Blocking: -835690
Blockedon: 835690
Status: Fixed (was: Untriaged)
I think this is fixed, Rakib? If not, feel free to reopen

Sign in to add a comment