Skip to content

Supress expected warning for test suite #604

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 22, 2025

Conversation

p-schlickmann
Copy link
Contributor

I believe we should supress the ERROR -- stack level too deep: nil that's currently happening in the test suite:

Screenshot 2025-07-22 at 14 40 29

The failure is intentional (we're exercising a SystemStackError on purpose), but the noisy log isn’t documented and has already confused contributors into thinking it's a real test failure (see #527).

@@ -15,13 +15,12 @@ class SolidQueue::FailedExecutionTest < ActiveSupport::TestCase
end

test "run job that fails with a SystemStackError (stack level too deep)" do
silence_on_thread_error_for(SystemStackError) do
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like we were already trying to do this, but with no success.

From my research, only exceptions who do not inherit StandardError are being logged.

ExpectedTestError inherits RuntimeError which inherits StandardError.

The problem with SystemStackError is that it inherits Exception - not StandardError - which leaks a log. Not 100% sure why though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh yes! I had forgotten we were trying to do this here, because it didn't work 😅

@rosa
Copy link
Member

rosa commented Jul 22, 2025

The intended goal for that test was to have a gigantic backtrace generated by a SystemStackError. It was added in #260 and improved in #261, when I hit this in our app in production. Does the new test fail if you remove the backtrace truncation?

if (limit = determine_backtrace_size_limit) && exception.backtrace.to_json.bytesize > limit
truncate_backtrace(exception.backtrace, limit)
else

@p-schlickmann
Copy link
Contributor Author

Hey @rosa

No, it doesn't fail. Nothing is really testing that backtrace truncation you mentioned, which made me notice something quite interesting, unrelated to my change:

The truncation depends on the limit set to the error column in the table solid_queue_failed_executions:

def determine_backtrace_size_limit
column = self.class.connection.schema_cache.columns_hash(self.class.table_name)["error"]
if column.limit.present?
column.limit - exception_class_name.bytesize - exception_message.bytesize - JSON_OVERHEAD
end
end

But solid_queue_failed_executions error column doesn't have limit set anywhere...

create_table "solid_queue_failed_executions", force: :cascade do |t|
t.bigint "job_id", null: false
t.text "error"
t.datetime "created_at", null: false
t.index [ "job_id" ], name: "index_solid_queue_failed_executions_on_job_id", unique: true
end

create_table "solid_queue_failed_executions", charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t|
t.bigint "job_id", null: false
t.text "error"
t.datetime "created_at", null: false
t.index ["job_id"], name: "index_solid_queue_failed_executions_on_job_id", unique: true
end

Truncation is not actually happening on main branch right now, because column.limit.present? is false.

@rosa
Copy link
Member

rosa commented Jul 22, 2025

The test failed without the truncation, but it's RDBMS-dependent. It's explained in 262c0c8
It fails for MySQL, which is what we use in production. The test didn't need to assert anything about the truncation because it'd fail directly without it.

@p-schlickmann
Copy link
Contributor Author

p-schlickmann commented Jul 22, 2025

The test failed without the truncation, but it's RDBMS-dependent. It's explained in 262c0c8 It fails for MySQL, which is what we use in production. The test didn't need to assert anything about the truncation because it'd fail directly without it.

Ohh that explains it, my bad, I only tested with SQLite and Postgres 😅.

When I didn't find limit: anywhere in the schema it was also kind of confusing 😝

@p-schlickmann
Copy link
Contributor Author

p-schlickmann commented Jul 22, 2025

@rosa I made a small change to pass the original backtrace to ExpectedTestError (see e70eef1)

Now the test does fail if truncation logic is removed and the database is MySQL 👍🏻

Screenshot 2025-07-22 at 16 13 51 Screenshot 2025-07-22 at 16 13 20

Copy link
Member

@rosa rosa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thank you so much! 🙏 🥳

@rosa rosa merged commit 0ba1b7e into rails:main Jul 22, 2025
43 checks passed
@p-schlickmann p-schlickmann deleted the supress-intended-warning branch July 22, 2025 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants