From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk1-x731.google.com (mail-qk1-x731.google.com [IPv6:2607:f8b0:4864:20::731]) by sourceware.org (Postfix) with ESMTPS id AE8B03945C30 for ; Tue, 26 May 2020 17:08:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org AE8B03945C30 Received: by mail-qk1-x731.google.com with SMTP id c12so2461858qkk.13 for ; Tue, 26 May 2020 10:08:42 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=KQ6bLCXRW0FWHMlLf8qS73MpSwPQecZfvIFTCESFtG8=; b=AC+BGt4cLkyIvA53NC0JCdwP6unTLahNf/yhpZ2LBK3G0fK8LNE39e0CU8BwjdqlDl i5QbM0LP0tKZm6ztEB3yJB/tcPfVS39CGXJGXQX/0W3YRiffzsvIqIwwHXbVH+YVngcd gW79N2I7lldBAWf0uHwT7VctBkUKh2pePjSdvFxnqRHNsF+HMaR/RM/f+KHTmwLTuAc2 zFgS/QoaGxnbkCngOwBpB1b5ilCouruw6vGyiU3mEpMtjCLqZSq5RyaHcUoTjH+9LWW/ Fijz4yyMIWYWbvf/FykvU/JRzOuwUX+WhzUKifU4riIs7kiBueJ/OPobjS53vsr4eEi5 XmQQ== X-Gm-Message-State: AOAM531ls++xPKg8rwpg5AkGcVejFjJXRWBNVIZJPkL8APHOQ54jMGuc EktmUiIcUuy6hiUUPTRajfbk6skdiuo= X-Google-Smtp-Source: ABdhPJxYtoP3m97ApsEZKNWCnJJ3X8gK4TTASpUw5NTORycG8bt4GmMf9DbK0U7Tw3DH8hxRhXQgIQ== X-Received: by 2002:a37:4ad2:: with SMTP id x201mr2554604qka.189.1590512921593; Tue, 26 May 2020 10:08:41 -0700 (PDT) Received: from [192.168.0.185] ([191.34.87.30]) by smtp.gmail.com with ESMTPSA id i94sm236708qtd.2.2020.05.26.10.08.39 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 26 May 2020 10:08:41 -0700 (PDT) Subject: Re: [PATCH, testsuite] Fix some duplicate test names To: Pedro Alves , gdb-patches@sourceware.org References: <20200526141848.22771-1-luis.machado@linaro.org> <5aa1d0a1-f674-efc8-bcb5-c71e36363e65@redhat.com> From: Luis Machado Message-ID: <2bf3d0a6-1f47-fbc9-5804-ca536da62746@linaro.org> Date: Tue, 26 May 2020 14:08:37 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <5aa1d0a1-f674-efc8-bcb5-c71e36363e65@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 26 May 2020 17:08:44 -0000 On 5/26/20 12:35 PM, Pedro Alves wrote: > Thanks! > > Some comments as I quickly skimmed the patch. Some of the issues > appear more than once, but I didn't point at all instances. > > On 5/26/20 3:18 PM, Luis Machado via Gdb-patches wrote: >> # Test that GDB manages caches correctly for tagged address. >> # Read from P2, >> -gdb_test "x p2" "$hex:\[\t \]+0x000004d2" >> +gdb_test "x p2" "$hex:\[\t \]+0x000004d2" "x p2" >> gdb_test_no_output "set variable i = 5678" >> # Test that *P2 is updated. >> -gdb_test "x p2" "$hex:\[\t \]+0x0000162e" >> +gdb_test "x p2" "$hex:\[\t \]+0x0000162e" "x p2 (updated)" >> > > NAK here -- these should be considered the same. See: > Heh, I seem to have forgotten that particular rule, and it is tempting to use it. Fixed now. > https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Do_not_use_.22tail_parentheses.22_on_test_messages > > If the detection machinery does not consider them duplicates, then it > should be fixed. It doesn't, but maybe it is due to the space before the parentheses? > >> gdb_test_multiple "break *test_ldr_literal_16" "break test_ldr_literal_16" { >> -re "Breakpoint.*at.* file .*$srcfile, line.*\r\n$gdb_prompt $" { >> - pass "break test_ldr_literal" >> + pass "break test_ldr_literal_16" >> } > > Why not "break *break test_ldr_literal_16"? I.e., the "*" is missing. > > But consider $gdb_test_name, and dropping the explicit test name in > the gdb_test_multiple call too: > > gdb_test_multiple "break *test_ldr_literal_16" "" { > -re "Breakpoint.*at.* file .*$srcfile, line.*\r\n$gdb_prompt $" { > pass $gdb_test_name > } > >> gdb_test_multiple "break *test_zero_cbnz" "break test_zero_cbnz" { >> -re "Breakpoint.*at.* file .*$srcfile, line.*\r\n$gdb_prompt $" { >> - pass "break test_ldr_literal" >> + pass "break test_ldr_literal in test_zero_cbnz" >> } > > This one shows why $gdb_test_name would be better -- here a fail caught > by gdb_test_multiple's internal patterns will issue a different message > compared to a PASS: > > FAIL: break test_zero_cbnz > PASS: break test_ldr_literal in test_zero_cbnz > Makes sense. I've fixed these and patched them up to use $gdb_test_name now. >> -gdb_test "print x" "$decimal = 45" >> +gdb_test "print x" "$decimal = 45" "validate setting a globa, 2nd time" >> > > Typo: "globa". > Fixed now. >> +++ b/gdb/testsuite/gdb.base/return2.exp >> @@ -50,7 +50,7 @@ proc return_1 { type } { >> gdb_test "print ${type}_resultval == testval.${type}_testval" ".* = 1" \ >> "${type} value returned successfully" >> gdb_test "print ${type}_resultval != ${type}_returnval" ".* = 1" \ >> - "validate result value not equal to program return value" >> + "validate result value not equal to program return value (${type})" >> } > > That issue with tail parens again. Fixed. Though I did not try to fix the testcase itself, which allows the command to be output as the test name, and those contain parentheses. I'd need to go over it and find meaningful names for the tests. > >> +gdb_test "p foo1_3 (a)" "Cannot resolve.*" \ >> + "pointer to pointer of wrong type (a)" >> +gdb_test "p foo1_3 (bp)" "Cannot resolve.*" \ >> + "pointer to pointer of wrong type (bp)" >> gdb_test "p foo1_4 (bp)" "= 14" "pointer to ancestor pointer" > > Ditto. > Fixed. >> +with_test_prefix "Strict type checking on" { >> + gdb_test "p foo1_type_check (123)" [format $error_str "foo1_type_check"] >> + gdb_test "p foo2_type_check (0, 1)" [format $error_str "foo2_type_check"] > > Lowercase "Strict". > Fixed. >> @@ -401,25 +401,25 @@ void do_frozen_tests () >> v1.nested.k = 9; >> /*: >> set_frozen V1 1 >> - mi_varobj_update * {} "update varobjs: nothing changed" >> - mi_check_varobj_value V1.i 1 "check V1.i: 1" >> - mi_check_varobj_value V1.nested.j 2 "check V1.nested.j: 2" >> - mi_check_varobj_value V1.nested.k 3 "check V1.nested.k: 3" >> + mi_varobj_update * {} "update varobjs: nothing changed, 2nd" >> + mi_check_varobj_value V1.i 1 "check V1.i: 1, 2nd" >> + mi_check_varobj_value V1.nested.j 2 "check V1.nested.j: 2, 2nd" >> + mi_check_varobj_value V1.nested.k 3 "check V1.nested.k: 3, 2nd" >> # Check that explicit update for elements of structures >> # works. >> # Update v1.j >> mi_varobj_update V1.nested.j {V1.nested.j} "update V1.nested.j" >> - mi_check_varobj_value V1.i 1 "check V1.i: 1" >> - mi_check_varobj_value V1.nested.j 8 "check V1.nested.j: 8" >> - mi_check_varobj_value V1.nested.k 3 "check V1.nested.k: 3" >> + mi_check_varobj_value V1.i 1 "check V1.i: 1, 3rd" >> + mi_check_varobj_value V1.nested.j 8 "check V1.nested.j: 8, 1st" >> + mi_check_varobj_value V1.nested.k 3 "check V1.nested.k: 3, 3rd" >> # Update v1.nested, check that children is updated. >> mi_varobj_update V1.nested {V1.nested.k} "update V1.nested" >> mi_check_varobj_value V1.i 1 "check V1.i: 1" >> - mi_check_varobj_value V1.nested.j 8 "check V1.nested.j: 8" >> + mi_check_varobj_value V1.nested.j 8 "check V1.nested.j: 8, 2nd" >> mi_check_varobj_value V1.nested.k 9 "check V1.nested.k: 9" >> # Update v1.i >> mi_varobj_update V1.i {V1.i} "update V1.i" >> - mi_check_varobj_value V1.i 7 "check V1.i: 7" >> + mi_check_varobj_value V1.i 7 "check V1.i: 7, 1st" > > Here, instead of the counting, it would seem to me better to use > with_test_prefix, like > > with_test_prefix "update *" { > ... > } > > with_test_prefix "update v1.j" { > ... > } > > with_test_prefix "v1.nested" { > ... > } I've switched to using with_test_prefix. I'll send a v2 shortly. Thanks!