Стек вызовов в информации об ошибке фонового задания #1487
Conversation
WalkthroughThe pull request introduces enhancements to error handling and stack trace management in the OneScript library. The changes focus on improving exception context capture, adding stack trace collection capabilities, and modifying the machine instance to create full call stacks. These modifications provide more detailed debugging information by enriching error handling mechanisms and enabling better introspection of execution frames. Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/OneScript.StandardLibrary/Tasks/BackgroundTask.cs(1 hunks)src/ScriptEngine/Machine/Contexts/StackTraceCollectionContext.cs(2 hunks)src/ScriptEngine/Machine/MachineInstance.cs(1 hunks)tests/tasks.os(2 hunks)
🔇 Additional comments (5)
src/ScriptEngine/Machine/Contexts/StackTraceCollectionContext.cs (2)
33-40: LGTM! Well-documented Count() method.The implementation is clean and the XML documentation clearly describes the purpose and return value.
48-48: LGTM! IsIndexed property enables frame access by index.The property correctly indicates that the collection supports indexed access.
src/OneScript.StandardLibrary/Tasks/BackgroundTask.cs (1)
114-114: LGTM! Enhanced error information with call stack.The change correctly captures the execution frames during exception handling, improving error diagnostics.
Run the following script to verify the integration with MachineInstance:
✅ Verification successful
Verified: GetExecutionFrames integration is correct and consistent
The method is properly integrated with MachineInstance and follows the established patterns for error handling and debugging across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that MachineInstance.GetExecutionFrames() is called during exception handling. # Test: Search for other usages of GetExecutionFrames() to ensure consistent error handling. rg -A 5 $'GetExecutionFrames'Length of output: 2262
tests/tasks.os (1)
268-282: LGTM! Comprehensive test coverage for call stack in error information.The test thoroughly validates:
- Stack trace collection type
- Number of frames
- Frame details including method name, line number, and module name
src/ScriptEngine/Machine/MachineInstance.cs (1)
2454-2454: LGTM! Ensures call stack is populated before access.The change correctly ensures that the full call stack is created before being returned, supporting the enhanced error reporting functionality.
src/ScriptEngine/Machine/Contexts/StackTraceCollectionContext.cs
Outdated
Show resolved
Hide resolved
2. В КоллекцияКадровСтекаВызовов добавлен метод Количество() 3. КоллекцияКадровСтекаВызовов сделана индексированной коллекцией
sfaqer
force-pushed
the
feature/bgTasksCallStack
branch
from
21ca850 to
01b44b5
Compare
| @@ -2451,6 +2451,7 @@ private IValue PopRawValue() | |||
|
|
|||
| public IList<ExecutionFrameInfo> GetExecutionFrames() | |||
| { | |||
| CreateFullCallstack(); | |||
There was a problem hiding this comment.
Вот это кажется перформанс критикал будет. На каждый конверт из ссылки в сырое значение делать стек...
There was a problem hiding this comment.
Я не совсем понимаю о чём ты говоришь.
Вот в приведённых вызовах который из это "конверт из ссылки в сырое значение" ?
There was a problem hiding this comment.
Прошу прощения, тупанул. UI гитхаба скрывает то, что написано выше, и отображает серым, а там метод PopRawValue. И я подумал, что мы в этот опкод добавили CreateFullStack
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/tasks.os (1)
275-290: Consider making the test more robust.While the test implementation is functional, there are a few areas that could be improved:
- The hard-coded line number
45makes the test fragile. IfПроцедураСИсключениемis modified, this test will break.- The assumption of exactly one frame in the stack trace might be too restrictive.
- Missing verification of error state before accessing the stack trace.
Consider this improved implementation:
Процедура ТестДолжен_ПроверитьЧтоВИнформацииОбОшибкеЕстьСтекВызовов() Экспорт Задание = ФоновыеЗадания.Выполнить(ЭтотОбъект, "ПроцедураСИсключением"); Задание.ОжидатьЗавершения(); + + юТест.ПроверитьРавенство(СостояниеФоновогоЗадания.ЗавершеноАварийно, Задание.Состояние); СтекВызовов = Задание.ИнформацияОбОшибке.ПолучитьСтекВызовов(); юТест.ПроверитьТип(СтекВызовов, "КоллекцияКадровСтекаВызовов"); - юТест.ПроверитьРавенство(СтекВызовов.Количество(), 1); + юТест.ПроверитьБольшеИлиРавно(СтекВызовов.Количество(), 1); юТест.ПроверитьРавенство(СтекВызовов[0].Метод, "ПроцедураСИсключением"); - юТест.ПроверитьРавенство(СтекВызовов[0].НомерСтроки, 45); + юТест.ПроверитьЗаполненность(СтекВызовов[0].НомерСтроки); юТест.ПроверитьРавенство(СтекВызовов[0].ИмяМодуля, ТекущийСценарий().Источник); КонецПроцедуры
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/tasks.os(2 hunks)
🔇 Additional comments (2)
tests/tasks.os (2)
27-27: LGTM!The new test is properly added to the test list, following the existing naming convention.
280-280: Verify stack trace collection functionality.Let's ensure that the stack trace collection is properly implemented in the codebase.
✅ Verification successful
Stack trace collection implementation verified
The stack trace collection is properly implemented in the core ExceptionInfoContext and consistently used across debug adapter, runtime engine, and test infrastructure. The implementation handles both successful and failure cases appropriately.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of stack trace collection in error handling. # Check for the stack trace collection method implementation ast-grep --pattern 'ПолучитьСтекВызовов' # Check for stack trace usage in error handling rg -l "ПолучитьСтекВызовов|GetStackTrace" -g "*.{cs,os}"Length of output: 417
Script:
#!/bin/bash # Check the implementation of stack trace collection in ExceptionInfoContext rg -A 10 "ПолучитьСтекВызовов|GetStackTrace" src/ScriptEngine/Machine/Contexts/ExceptionInfoContext.csLength of output: 577
| @@ -2451,6 +2451,7 @@ private IValue PopRawValue() | |||
|
|
|||
| public IList<ExecutionFrameInfo> GetExecutionFrames() | |||
| { | |||
| CreateFullCallstack(); | |||
There was a problem hiding this comment.
Прошу прощения, тупанул. UI гитхаба скрывает то, что написано выше, и отображает серым, а там метод PopRawValue. И я подумал, что мы в этот опкод добавили CreateFullStack
|
@sfaqer просьба пофиксить вот эти 2 теста https://build.oscript.io/blue/organizations/jenkins/1Script/detail/PR-1487/3/tests |
…фонового задания, НомерСтроки теперь проверяется на заполненность, чтобы тест не падал при добавлении новых тестов.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/tasks.os (1)
275-290: Consider enhancing test coverage with complex call stacks.The current test verifies a simple call stack with one frame. Consider adding test cases that verify:
- Nested method calls resulting in deeper call stacks
- Additional frame properties if available in the
КадрСтекаВызововtypeExample enhancement:
+Процедура ВложеннаяПроцедура() Экспорт + ВызватьИсключение "Ошибка из вложенной процедуры"; +КонецПроцедуры + +Процедура ПроцедураСВложеннымИсключением() Экспорт + ВложеннаяПроцедура(); +КонецПроцедуры + Процедура ТестДолжен_ПроверитьЧтоВИнформацииОбОшибкеЕстьСтекВызовов() Экспорт - Задание = ФоновыеЗадания.Выполнить(ЭтотОбъект, "ПроцедураСИсключением"); + // Проверка простого стека вызовов + Задание = ФоновыеЗадания.Выполнить(ЭтотОбъект, "ПроцедураСИсключением"); Задание.ОжидатьЗавершения(); СтекВызовов = Задание.ИнформацияОбОшибке.ПолучитьСтекВызовов(); @@ ... @@ + // Проверка вложенного стека вызовов + Задание = ФоновыеЗадания.Выполнить(ЭтотОбъект, "ПроцедураСВложеннымИсключением"); + Задание.ОжидатьЗавершения(); + + СтекВызовов = Задание.ИнформацияОбОшибке.ПолучитьСтекВызовов(); + юТест.ПроверитьРавенство(СтекВызовов.Количество(), 2); + юТест.ПроверитьРавенство(СтекВызовов[0].Метод, "ВложеннаяПроцедура"); + юТест.ПроверитьРавенство(СтекВызовов[1].Метод, "ПроцедураСВложеннымИсключением");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/tasks.os(2 hunks)
🔇 Additional comments (2)
tests/tasks.os (2)
27-27: LGTM!The test registration follows the established naming convention and is appropriately grouped with other error-related tests.
275-290: LGTM! Well-structured test implementation.The test is well-implemented with comprehensive assertions covering the call stack type, frame count, and frame properties.
=) |
Summary by CodeRabbit
New Features
Tests
These updates enhance error reporting and stack trace management capabilities, providing users with better insights during exceptions.