Подключены и обновлены фикстуры, фикс чтения предопределенных#626
Merged
Conversation
📝 WalkthroughWalkthroughRegisters two shallow ChangesSSL 3.2 Fixture Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/test/java/com/github/_1c_syntax/bsl/test_utils/MDTestUtils.java (1)
135-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate the new public API with jspecify nullness.
fixturePostfixis nullable by design here, but the signature leaves that contract implicit. Please expose it with@Nullable/@NonNullat the method boundary. As per coding guidelines, "Use@Nullableand@NonNullannotations from org.jspecify for null safety."Proposed fix
+import org.jspecify.annotations.NonNull; +import org.jspecify.annotations.Nullable; ... - public static void regenerateFixture(String examplePackName, String mdoRef, boolean isEdt, String fixturePostfix) throws java.io.IOException { + public static void regenerateFixture(`@NonNull` String examplePackName, + `@NonNull` String mdoRef, + boolean isEdt, + `@Nullable` String fixturePostfix) throws java.io.IOException {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/github/_1c_syntax/bsl/test_utils/MDTestUtils.java` around lines 135 - 143, The new public API in MDTestUtils.regenerateFixture has an implicit nullable contract for fixturePostfix, so annotate the method boundary with org.jspecify nullness annotations. Mark fixturePostfix as `@Nullable` and add `@NonNull` to the non-null parameters/return contract as appropriate, keeping the signature explicit and consistent with the rest of the public test utility API.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/test/java/com/github/_1c_syntax/bsl/mdclasses/ConfigurationTest.java`:
- Around line 225-227: Replace the tautological size check in ConfigurationTest
with a real expected-size assertion for the ssl_3_2 plain children fixture: in
the test around cf.getPlainChildren(), assert against a fixed expected count (or
a separately defined fixture-specific constant) instead of recomputing the size
from the same collection. Apply the same correction to the other matching test
near the second plain-children assertion, and keep the
SupportVariant.NOT_EDITABLE check unchanged.
In `@src/test/java/com/github/_1c_syntax/bsl/test_utils/MDTestUtils.java`:
- Around line 139-140: MDOReader.read(...) can return null, so the current
createJson(...) path in MDTestUtils should fail fast with a clear assertion
instead of allowing a later NPE. Update the logic around mdo and mdoRef to
explicitly check the result of MDOReader.read(...) and stop immediately with a
descriptive failure when the lookup misses, keeping the existing createJson(...)
flow only for non-null MDOs.
---
Nitpick comments:
In `@src/test/java/com/github/_1c_syntax/bsl/test_utils/MDTestUtils.java`:
- Around line 135-143: The new public API in MDTestUtils.regenerateFixture has
an implicit nullable contract for fixturePostfix, so annotate the method
boundary with org.jspecify nullness annotations. Mark fixturePostfix as
`@Nullable` and add `@NonNull` to the non-null parameters/return contract as
appropriate, keeping the signature explicit and consistent with the rest of the
public test utility API.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1916b3a9-0d89-488e-9598-84798cb0529d
⛔ Files ignored due to path filters (66)
src/test/resources/ext/designer/ssl_3_1is excluded by!src/test/resources/**src/test/resources/ext/designer/ssl_3_2is excluded by!src/test/resources/**src/test/resources/ext/edt/ssl_3_1is excluded by!src/test/resources/**src/test/resources/ext/edt/ssl_3_2is excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/Catalogs.ВерсииФайлов.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/Catalogs.ВерсииФайлов_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/ExchangePlans.ОбновлениеИнформационнойБазы.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/ExchangePlans.ОбновлениеИнформационнойБазы_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/Roles.БазовыеПраваБСП.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/BusinessProcesses.Задание.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/BusinessProcesses.Задание_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/Catalogs.ВерсииФайлов.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/Catalogs.ВерсииФайлов_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/Catalogs.Заметки.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/Catalogs.Заметки_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/ChartsOfCharacteristicTypes.ДополнительныеРеквизитыИСведения.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/ChartsOfCharacteristicTypes.ДополнительныеРеквизитыИСведения_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/CommandGroups.Печать.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/CommonAttributes.ОбластьДанныхВспомогательныеДанные.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/CommonCommands.ОтправитьПисьмо.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/CommonCommands.ОтправитьПисьмо_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/CommonForms.Вопрос.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/CommonForms.Вопрос_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/CommonModules.АвтономнаяРабота.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/CommonModules.АвтономнаяРабота_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/CommonPictures.GoogleMaps.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/CommonTemplates.СтруктураПодчиненности.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/Configuration.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/Configuration_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/Constants.ЗаголовокСистемы.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/DataProcessors.ЗагрузкаКурсовВалют.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/DataProcessors.ЗагрузкаКурсовВалют_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/DefinedTypes.ВладелецФайлов.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/DocumentJournals.Взаимодействия.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/DocumentJournals.Взаимодействия_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/Documents.Анкета.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/Documents.Анкета_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/Enums.СтатусыОбработчиковОбновления.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/Enums.СтатусыОбработчиковОбновления_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/EventSubscriptions.ВариантыОтчетовПередУдалениемИдентификатораОбъектаМетаданных.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/ExchangePlans.ОбновлениеИнформационнойБазы.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/ExchangePlans.ОбновлениеИнформационнойБазы_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/FilterCriteria.СвязанныеДокументы.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/FilterCriteria.СвязанныеДокументы_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/FunctionalOptions.ИспользоватьАнкетирование.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/FunctionalOptionsParameters.ТипВерсионируемогоОбъекта.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/InformationRegisters.СклоненияПредставленийОбъектов.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/InformationRegisters.СклоненияПредставленийОбъектов_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/InformationRegisters.ЭлектронныеПодписи.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/InformationRegisters.ЭлектронныеПодписи_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/Languages.Русский.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/Reports.АнализВерсийОбъектов.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/Reports.АнализВерсийОбъектов_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/Roles.БазовыеПраваБСП.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/ScheduledJobs.ОбновлениеАгрегатов.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/SessionParameters.ТекущийПользователь.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/SettingsStorages.ХранилищеВариантовОтчетов.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/SettingsStorages.ХранилищеВариантовОтчетов_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/StyleItems.ВажнаяНадписьШрифт.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/StyleItems.ВидДняПроизводственногоКалендаряВоскресеньеЦвет.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/Subsystems.Администрирование.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/Tasks.ЗадачаИсполнителя.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/Tasks.ЗадачаИсполнителя_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/WebServices.EnterpriseDataExchange_1_0_1_1.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/WebServices.EnterpriseDataExchange_1_0_1_1_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_2/XDTOPackages.ApdexExport_1_0_0_4.jsonis excluded by!src/test/resources/**
📒 Files selected for processing (41)
.gitmodulessrc/main/java/com/github/_1c_syntax/bsl/reader/common/converter/PredefinedValueConverter.javasrc/test/java/com/github/_1c_syntax/bsl/examples/ValueTypeTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdclasses/ConfigurationTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdclasses/MDClassesTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdclasses/helpers/RightsTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdo/BusinessProcessTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdo/CatalogTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdo/ChartOfCharacteristicTypesTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdo/CommandGroupTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdo/CommonAttributeTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdo/CommonCommandTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdo/CommonFormTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdo/CommonModuleTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdo/CommonPictureTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdo/CommonTemplateTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdo/ConstantTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdo/DataProcessorTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdo/DefinedTypeTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdo/DocumentJournalTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdo/DocumentTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdo/EnumTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdo/EventSubscriptionTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdo/ExchangePlanTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdo/FilterCriterionTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdo/FunctionalOptionTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdo/FunctionalOptionsParameterTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdo/InformationRegisterTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdo/LanguageTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdo/ReportTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdo/RoleTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdo/ScheduledJobTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdo/SessionParameterTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdo/SettingsStorageTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdo/StyleItemTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdo/SubsystemTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdo/TasksTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdo/WebServiceTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdo/XDTOPackageTest.javasrc/test/java/com/github/_1c_syntax/bsl/reader/MDOReaderTest.javasrc/test/java/com/github/_1c_syntax/bsl/test_utils/MDTestUtils.java
Comment on lines
+225
to
+227
| assertThat(cf.getPlainChildren()) | ||
| .hasSize(nonPredefinedCount(cf.getPlainChildren()) + predefinedCount32(cf.getPlainChildren())) | ||
| .allMatch(md -> md.getSupportVariant().equals(SupportVariant.NOT_EDITABLE)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restore a real expected-size assertion for ssl_3_2 plain children.
Both assertions are tautologies: they compare cf.getPlainChildren().size() to a sum recomputed from that same list. If the parser drops or adds children, these tests still pass, so the new ssl_3_2 coverage no longer guards the fixture contract.
Also applies to: 433-435
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/test/java/com/github/_1c_syntax/bsl/mdclasses/ConfigurationTest.java`
around lines 225 - 227, Replace the tautological size check in ConfigurationTest
with a real expected-size assertion for the ssl_3_2 plain children fixture: in
the test around cf.getPlainChildren(), assert against a fixed expected count (or
a separately defined fixture-specific constant) instead of recomputing the size
from the same collection. Apply the same correction to the other matching test
near the second plain-children assertion, and keep the
SupportVariant.NOT_EDITABLE check unchanged.
Comment on lines
+139
to
+140
| var mdo = MDOReader.read(configurationPath, mdoRef, MDCReadSettings.DEFAULT); | ||
| var json = createJson(mdo); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Fail fast when the MDO lookup misses.
MDOReader.read(...) is nullable, and the existing reader tests already exercise null for unknown references. A typo in mdoRef will currently fall through to createJson(...) and blow up as a bare NPE.
Proposed fix
var mdo = MDOReader.read(configurationPath, mdoRef, MDCReadSettings.DEFAULT);
+ if (mdo == null) {
+ throw new IllegalArgumentException("MDO not found: " + mdoRef + " in " + configurationPath);
+ }
var json = createJson(mdo);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var mdo = MDOReader.read(configurationPath, mdoRef, MDCReadSettings.DEFAULT); | |
| var json = createJson(mdo); | |
| var mdo = MDOReader.read(configurationPath, mdoRef, MDCReadSettings.DEFAULT); | |
| if (mdo == null) { | |
| throw new IllegalArgumentException("MDO not found: " + mdoRef + " in " + configurationPath); | |
| } | |
| var json = createJson(mdo); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/test/java/com/github/_1c_syntax/bsl/test_utils/MDTestUtils.java` around
lines 139 - 140, MDOReader.read(...) can return null, so the current
createJson(...) path in MDTestUtils should fail fast with a clear assertion
instead of allowing a later NPE. Update the logic around mdo and mdoRef to
explicitly check the result of MDOReader.read(...) and stop immediately with a
descriptive failure when the lookup misses, keeping the existing createJson(...)
flow only for non-null MDOs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Описание
Связанные задачи
Closes: #584
Чеклист
Общие
gradlew precommit)Дополнительно
Summary by CodeRabbit
New Features
Bug Fixes
Tests