Make test failure source locations lazy #12260

GregoryTravis · 2025-02-11T21:55:57Z

Replaces eager calls to Test.get_source_location to lazy calls to Frame_Hider.get_top_stack_frame, resulting in a 2x speedup for Base_Tests.

Test utilities wrap their contents in Frame_Hider.hide, and wrap callbacks in Frame_Hider.unhide. These marker frames are used to skip test utility frames to find the real user-level location of the test failure.

The frames_to_skip parameter is no longer needed, since the number of frames to skip is computed dynamically on test failure.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
Unit tests have been written where possible.
If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,
or the Snowflake database integration, a run of the Extra Tests has been scheduled.
- If applicable, it is suggested to paste a link to a successful run of the Extra Tests.

JaroslavTulach · 2025-02-13T04:38:36Z

distribution/lib/Standard/Test/0.0.0-dev/src/Frame_Hider.enso

+from Standard.Base import all
+import Standard.Base.Runtime.Ref.Ref
+
+## PRIVATE


Could this module be private? Probably it couldn't be.

JaroslavTulach · 2025-02-13T04:39:36Z

distribution/lib/Standard/Test/0.0.0-dev/src/Main.enso

@@ -27,6 +27,8 @@ export project.Extensions.should_succeed

 export project.Faker.Faker

+export project.Frame_Hider.Frame_Hider


Ech, probably the Frame_Hider couldn't be private.

JaroslavTulach · 2025-02-13T04:44:48Z

distribution/lib/Standard/Test/0.0.0-dev/src/Frame_Hider.enso

+         test_utility test_predicate x =
+             ...
+             Frame_Hider.unhide_1 test_predicate x
+    unhide_1 f x =


Wow, this is so non-FPish!

I mean:

Enso does support currying

this kind constructs (Function1, Function2, etc.) are more likely to find in language that don't support currying (Java, Scala, Kotlin)

Of course I wanted to curry it, but it seems not to work; perhaps there is something I'm doing wrong.

Demonstrated below: for a two-argument function, only the version with two explicit arguments has wrap in its call stack:

foo a b = st = Runtime.get_stack_trace st.map IO.println IO.println "====" a + b wrap_0 f = f wrap_1 f x = f x wrap_2 f x y = f x y main = sp <| foo 1 2 sp <| wrap_0 foo 1 2 sp <| wrap_1 foo 1 2 sp <| wrap_2 foo 1 2

Results:

M:dee wip/gmt/12215-slow-stack-frame$ g (Stack_Trace_Element.Value 'dbbd.foo' (Source_Location dbbd.enso:126:10-32)) (Stack_Trace_Element.Value 'dbbd.main<arg-1>' (Source_Location dbbd.enso:141:11-17)) (Stack_Trace_Element.Value 'dbbd.main' (Source_Location dbbd.enso:141:5-17)) ==== 3 (Stack_Trace_Element.Value 'dbbd.foo' (Source_Location dbbd.enso:126:10-32)) (Stack_Trace_Element.Value 'dbbd.main<arg-1>' (Source_Location dbbd.enso:142:11-24)) (Stack_Trace_Element.Value 'dbbd.main' (Source_Location dbbd.enso:142:5-24)) ==== 3 (Stack_Trace_Element.Value 'dbbd.foo' (Source_Location dbbd.enso:126:10-32)) (Stack_Trace_Element.Value 'dbbd.main<arg-1>' (Source_Location dbbd.enso:143:11-24)) (Stack_Trace_Element.Value 'dbbd.main' (Source_Location dbbd.enso:143:5-24)) ==== 3 (Stack_Trace_Element.Value 'dbbd.foo' (Source_Location dbbd.enso:126:10-32)) (Stack_Trace_Element.Value 'dbbd.wrap_2' (Source_Location dbbd.enso:138:5-9)) (Stack_Trace_Element.Value 'dbbd.main<arg-1>' (Source_Location dbbd.enso:144:11-24)) (Stack_Trace_Element.Value 'dbbd.main' (Source_Location dbbd.enso:144:5-24)) ==== 3

In addition to this unfortunate code, I have had to do this in some tests that use callbacks:

- tester = expect_column_names ["A", "1980-01-01", "Column 1", "5.3", "True"] + tester t = expect_column_names ["A", "1980-01-01", "Column 1", "5.3", "True"] t problems = [Duplicate_Output_Column_Names.Error ["A", "A", "A"]] Problems.test_problem_handling action problems tester

(tester is invoked within an unhide call in test_problem_handling.)

It makes me sad, but it was necessary.

Following program:

from Standard.Base import all sp = IO.println foo a b = st = Runtime.get_stack_trace st.map IO.println IO.println "====" a + b wrap_l ~a = a main = sp <| foo 1 2 sp <| wrap_l <| foo 1 2 sp <| wrap_l <| foo 1 2 sp <| wrap_l <| foo 1 2

does include wrap_l in all the invocations:

(Stack_Trace_Element.Value 'g.foo' (Source_Location g.enso:7:10-32)) (Stack_Trace_Element.Value 'g.main<arg-1>' (Source_Location g.enso:16:11-17)) (Stack_Trace_Element.Value 'g.main' (Source_Location g.enso:16:5-17)) ==== 3 (Stack_Trace_Element.Value 'g.foo' (Source_Location g.enso:7:10-32)) (Stack_Trace_Element.Value 'g.main<arg-1>' (Source_Location g.enso:17:21-27)) (Stack_Trace_Element.Value 'g.wrap_l' (Source_Location g.enso:13:5-5)) (Stack_Trace_Element.Value 'g.main<arg-1>' (Source_Location g.enso:17:11-27)) (Stack_Trace_Element.Value 'g.main' (Source_Location g.enso:17:5-27)) ==== 3 (Stack_Trace_Element.Value 'g.foo' (Source_Location g.enso:7:10-32)) (Stack_Trace_Element.Value 'g.main<arg-1>' (Source_Location g.enso:18:21-27)) (Stack_Trace_Element.Value 'g.wrap_l' (Source_Location g.enso:13:5-5)) (Stack_Trace_Element.Value 'g.main<arg-1>' (Source_Location g.enso:18:11-27)) (Stack_Trace_Element.Value 'g.main' (Source_Location g.enso:18:5-27)) ==== 3 (Stack_Trace_Element.Value 'g.foo' (Source_Location g.enso:7:10-32)) (Stack_Trace_Element.Value 'g.main<arg-1>' (Source_Location g.enso:19:21-27)) (Stack_Trace_Element.Value 'g.wrap_l' (Source_Location g.enso:13:5-5)) (Stack_Trace_Element.Value 'g.main<arg-1>' (Source_Location g.enso:19:11-27)) (Stack_Trace_Element.Value 'g.main' (Source_Location g.enso:19:5-27)) ==== 3

The only difference is that one has to use wrap_l <| instead of just wrap_l.

But for a callback, wrap_l is not included, as seen below. This is why I added unhide_1 and unhide_2 for -- the case when user-supplied callbacks are used to implement the value testers.

sp <| (wrap_l <| foo 1) 2

(Stack_Trace_Element.Value 'jt.foo' (Source_Location jt.enso:6:10-32)) (Stack_Trace_Element.Value 'jt.main<arg-1>' (Source_Location jt.enso:18:11-29)) (Stack_Trace_Element.Value 'jt.main' (Source_Location jt.enso:18:5-29)) ==== 3

JaroslavTulach · 2025-02-13T04:46:23Z

distribution/lib/Standard/Test/0.0.0-dev/src/Frame_Hider.enso

+   user-supplied callback, depending on the number of arguments. If one test
+   utility forwards a callback to another test utility, the callback should be
+   wrapped at the forward site as well.
+type Frame_Hider


Why is Frame_Hider a type, when it doesn't have a constructor? Putting its static methods as module methods would work as well.

Strangely, I am getting this error when I try this, but only in some tests -- other tests run correctly.

- [FAILED] should work as shown in the doc examples [472ms] Reason: An unexpected panic was thrown: (Syntax_Error.Error 'Type for ResolvedType(Concrete(CompilerContext.Module{module=Module[Standard.Test.Frame_Hider]}),Type(Frame_Hider,Seq(),Seq(),false)) is null') at <enso> Util.expect_column_names<arg-0>(/Users/gmt/dev/enso/enso/test/Table_Tests/src/Common_Table_Operations/Util.enso:10:54-64) at <enso> Util.expect_column_names(/Users/gmt/dev/enso/enso/test/Table_Tests/src/Common_Table_Operations/Util.enso:10-13)

Unexpected panic is an suspicious outcome - consider reporting it as a separate issue. If there is supposed to be an error, it shouldn't be unexpected panic. But yeah, such situations happen when we tease the compiler with unusual coding approaches.

After this PR, I'll create a task to remove the type and if it is still failing I'll report the bug.

JaroslavTulach · 2025-02-13T04:51:04Z

distribution/lib/Standard/Test/0.0.0-dev/src/Frame_Hider.enso

+
+## PRIVATE
+
+   Frame_Hider is used in test utilities to hide uninteresting stack frames from


The name Frame_Hider feels "too strong" to me. The verb "to hide" feel negative. But we want users of our APIs to feel good!

Do you have some prior art for selecting such a name?

When thinking about an alternative a phase shadow stack come to my mind:

The shadow stack itself is a second, separate stack that "shadows" the program call stack.

Maybe "shadow stack" would be misleading as well, but "to shadow" certainly feels more positive than "to hide" ;-)

I am not sure if this is shadowing -- that implies a duplicate / parallel stack. What we are really doing is removing some frames from the stack before picking the top frame.

I originally called this Omitter, maybe that's less hostile? Frame_Omitter perhaps?

Frame_Tidy? Frame_Grooming? Frame_Refining? Frame_Cleansing?

To be honest looking at all the possible names, Frame_Hider still seems most clear (IMHO) about what it is doing, I don't really share the feeling that it is 'negative' in any way...

It is an internal API anyway - mostly for library developers so the naming can probably be a bit less stringent.

Internal API would be private. E.g. no, this is not an internal API.

But it is less public than GUI-facing things.

It is marked as PRIVATE and meant to be used by library developers (advanced users) not by every user. There is some difference.

What is an API?

Everything somebody else can depend on!

The PRIVATE comment is mostly irrelevant in deciding whether someone can depend on the Frame_Hider name or not. It is just a comment.

There is some difference.

Even users of the IDE can depend and use the Frame_Hider. The PRIVATE comment just makes it harder for them to find it. But we have no way to know whether or not they found it!

The only way to make sure someone cannot (easily) depend on an element in a Standard.Test library is to use private keyword.

New gate for stdlib API check #12223

API check for all standard libraries #12285

Once a check for Standard.Test API stability is in, the "difference" will be clear. PRIVATE comment is unimportant from API perspective.

JaroslavTulach · 2025-02-13T04:52:46Z

test/Table_Tests/src/In_Memory/Table_Spec.enso

@@ -703,8 +703,8 @@ add_specs suite_builder =
            actual.column_names . should_equal ["col0", "col_between", "col1"]

    suite_builder.group "Use First Row As Names" group_builder->
-        expect_column_names names table =
-            table.columns . map .name . should_equal names frames_to_skip=2
+        expect_column_names names table = Frame_Hider.hide <|


Yeah, removing frames_to_skip=2 is certainly an improvement.

JaroslavTulach

Opinions inline...

JaroslavTulach · 2025-02-17T14:50:54Z

distribution/lib/Standard/Test/0.0.0-dev/src/Frame_Hider.enso

+
+## PRIVATE
+
+   Frame_Hider is used in test utilities to hide uninteresting stack frames from


Frame_Tidy? Frame_Grooming? Frame_Refining? Frame_Cleansing?

JaroslavTulach · 2025-02-17T14:53:14Z

distribution/lib/Standard/Test/0.0.0-dev/src/Frame_Hider.enso

+   user-supplied callback, depending on the number of arguments. If one test
+   utility forwards a callback to another test utility, the callback should be
+   wrapped at the forward site as well.
+type Frame_Hider


Unexpected panic is an suspicious outcome - consider reporting it as a separate issue. If there is supposed to be an error, it shouldn't be unexpected panic. But yeah, such situations happen when we tease the compiler with unusual coding approaches.

JaroslavTulach · 2025-02-17T15:02:58Z

distribution/lib/Standard/Test/0.0.0-dev/src/Frame_Hider.enso

+         test_utility test_predicate x =
+             ...
+             Frame_Hider.unhide_1 test_predicate x
+    unhide_1 f x =


Following program:

from Standard.Base import all sp = IO.println foo a b = st = Runtime.get_stack_trace st.map IO.println IO.println "====" a + b wrap_l ~a = a main = sp <| foo 1 2 sp <| wrap_l <| foo 1 2 sp <| wrap_l <| foo 1 2 sp <| wrap_l <| foo 1 2

does include wrap_l in all the invocations:

(Stack_Trace_Element.Value 'g.foo' (Source_Location g.enso:7:10-32)) (Stack_Trace_Element.Value 'g.main<arg-1>' (Source_Location g.enso:16:11-17)) (Stack_Trace_Element.Value 'g.main' (Source_Location g.enso:16:5-17)) ==== 3 (Stack_Trace_Element.Value 'g.foo' (Source_Location g.enso:7:10-32)) (Stack_Trace_Element.Value 'g.main<arg-1>' (Source_Location g.enso:17:21-27)) (Stack_Trace_Element.Value 'g.wrap_l' (Source_Location g.enso:13:5-5)) (Stack_Trace_Element.Value 'g.main<arg-1>' (Source_Location g.enso:17:11-27)) (Stack_Trace_Element.Value 'g.main' (Source_Location g.enso:17:5-27)) ==== 3 (Stack_Trace_Element.Value 'g.foo' (Source_Location g.enso:7:10-32)) (Stack_Trace_Element.Value 'g.main<arg-1>' (Source_Location g.enso:18:21-27)) (Stack_Trace_Element.Value 'g.wrap_l' (Source_Location g.enso:13:5-5)) (Stack_Trace_Element.Value 'g.main<arg-1>' (Source_Location g.enso:18:11-27)) (Stack_Trace_Element.Value 'g.main' (Source_Location g.enso:18:5-27)) ==== 3 (Stack_Trace_Element.Value 'g.foo' (Source_Location g.enso:7:10-32)) (Stack_Trace_Element.Value 'g.main<arg-1>' (Source_Location g.enso:19:21-27)) (Stack_Trace_Element.Value 'g.wrap_l' (Source_Location g.enso:13:5-5)) (Stack_Trace_Element.Value 'g.main<arg-1>' (Source_Location g.enso:19:11-27)) (Stack_Trace_Element.Value 'g.main' (Source_Location g.enso:19:5-27)) ==== 3

The only difference is that one has to use wrap_l <| instead of just wrap_l.

JaroslavTulach · 2025-02-19T06:09:40Z

Every time I am executing runEngineDistribution --run test/Base_Tests and waiting for more than five minutes, I am praying for this PR to be finally integrated. Is there any chance we will get the execution of test/Base_Tests below three minutes this week? What's blocking the integration?

GregoryTravis · 2025-02-19T15:21:53Z

Every time I am executing runEngineDistribution --run test/Base_Tests and waiting for more than five minutes, I am praying for this PR to be finally integrated. Is there any chance we will get the execution of test/Base_Tests below three minutes this week? What's blocking the integration?

Just waiting on an engine approval.

JaroslavTulach

There may be things I don't fully adore present in this PR
I have expressed my opinion in a previous review
However none of my opinions is critical enough to prevent the integration
If you are OK maintaining the API, go on and integrate
Approving from engine point of view

GregoryTravis added 30 commits February 5, 2025 14:03

wip

12632d6

nesting

007f068

wip

c4438e7

wip

f926e8b

all in extensions and test

461f7b2

wip

39bcd9b

convert all

5c5a9bd

all fts

1fd5911

no gsl

ff9c67a

wip

ca2c0ce

merge

10e6212

simplify, no obj

8f26d0a

wip

20072e9

wip

b8afddf

fix signatures

75cfe92

wip

10814db

into Main

c3635b0

wip

c4cdca9

put gsl back for with_retries

7fff45f

gsl test back

11ba0f8

wip

429fd69

wip

2cdfdf9

disambiguate

e9d78e3

unomitting

c8af61b

more tests

9828302

goofy but works

e524998

fold

0eb6018

log

02dd307

problems

26ed0f6

Merge branch 'develop' into wip/gmt/12215-slow-stack-frame

be6f652

JaroslavTulach reviewed Feb 13, 2025

View reviewed changes

GregoryTravis added 4 commits February 13, 2025 13:49

Merge branch 'develop' into wip/gmt/12215-slow-stack-frame

4b77e2b

uncurry testers

496c2bd

uncurry testers

c75bc3f

uncurry testers

d1bed32

GregoryTravis requested a review from JaroslavTulach February 13, 2025 21:06

GregoryTravis added 2 commits February 13, 2025 16:07

remove debug

d3e668c

wip

82b40c0

unfurl-links bot mentioned this pull request Feb 14, 2025

Let EnsoMultiValue.to_text delegate to first type to_text #11827

Closed

JaroslavTulach reviewed Feb 17, 2025

View reviewed changes

GregoryTravis added 2 commits February 17, 2025 11:24

Merge branch 'develop' into wip/gmt/12215-slow-stack-frame

3f01593

Merge branch 'develop' into wip/gmt/12215-slow-stack-frame

d1acc0d

GregoryTravis requested a review from JaroslavTulach February 17, 2025 16:58

enso-bot bot mentioned this pull request Feb 18, 2025

State and Contexts are not preserved through the Polyglot boundary #7117

Closed

JaroslavTulach approved these changes Feb 19, 2025

View reviewed changes

GregoryTravis added 2 commits February 19, 2025 10:45

Merge branch 'develop' into wip/gmt/12215-slow-stack-frame

fd369d2

revert local-only test fix

5da71e2

GregoryTravis added CI: Ready to merge This PR is eligible for automatic merge CI: Clean build required CI runners will be cleaned before and after this PR is built. labels Feb 19, 2025

GregoryTravis added 4 commits February 20, 2025 12:14

Merge branch 'develop' into wip/gmt/12215-slow-stack-frame

5fb6928

remove test/ from filenames

f803c6e

Merge branch 'develop' into wip/gmt/12215-slow-stack-frame

d0d085c

platform agnostic

8f07385

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make test failure source locations lazy #12260

Make test failure source locations lazy #12260

GregoryTravis commented Feb 11, 2025 •

edited

Loading

JaroslavTulach Feb 13, 2025 •

edited

Loading

JaroslavTulach Feb 13, 2025

JaroslavTulach Feb 13, 2025 •

edited

Loading

GregoryTravis Feb 13, 2025

GregoryTravis Feb 13, 2025

JaroslavTulach Feb 17, 2025

GregoryTravis Feb 17, 2025

JaroslavTulach Feb 13, 2025

GregoryTravis Feb 13, 2025 •

edited

Loading

JaroslavTulach Feb 17, 2025

GregoryTravis Feb 17, 2025

JaroslavTulach Feb 13, 2025

GregoryTravis Feb 13, 2025

JaroslavTulach Feb 17, 2025

radeusgd Feb 17, 2025 •

edited

Loading

JaroslavTulach Feb 18, 2025

radeusgd Feb 18, 2025

JaroslavTulach Feb 19, 2025

JaroslavTulach Feb 13, 2025

JaroslavTulach left a comment

JaroslavTulach Feb 17, 2025

JaroslavTulach Feb 17, 2025

JaroslavTulach Feb 17, 2025

JaroslavTulach commented Feb 19, 2025

GregoryTravis commented Feb 19, 2025

JaroslavTulach left a comment

		@@ -27,6 +27,8 @@ export project.Extensions.should_succeed

		export project.Faker.Faker

		export project.Frame_Hider.Frame_Hider


		## PRIVATE

		Frame_Hider is used in test utilities to hide uninteresting stack frames from

Make test failure source locations lazy #12260

Are you sure you want to change the base?

Make test failure source locations lazy #12260

Conversation

GregoryTravis commented Feb 11, 2025 • edited Loading

Checklist

JaroslavTulach Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GregoryTravis Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radeusgd Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach commented Feb 19, 2025

GregoryTravis commented Feb 19, 2025

JaroslavTulach left a comment

Choose a reason for hiding this comment

GregoryTravis commented Feb 11, 2025 •

edited

Loading

JaroslavTulach Feb 13, 2025 •

edited

Loading

JaroslavTulach Feb 13, 2025 •

edited

Loading

GregoryTravis Feb 13, 2025 •

edited

Loading

radeusgd Feb 17, 2025 •

edited

Loading