Рефакторинг тестов и фиксы#637
Conversation
2. хранение пути для макета изменено с Path на URI 3. скорректировано заполнение режима совместимости расширений 4. скорректирован формат длины для стандартных реквизитов кода и номера 5. реализовано чтение стилей и интерфейсов обычного приложения для формата едт 6. скорректированы конверторы для сериализации в json для тестов
2. добавлены тесты и фикстуры для дочерних
2. добавлены тесты для форм
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR adds explicit ChangesProduction code changes
Test fixture infrastructure and test suite rewrite
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
Test Results 486 files + 84 486 suites +84 11m 43s ⏱️ + 2m 55s Results for commit 2c7c3f5. ± Comparison against base commit b55ee1f. This pull request removes 140 and adds 994 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/com/github/_1c_syntax/bsl/test_utils/TestURIConverter.java (1)
33-68: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse slash-separated URI fragments here
TestURIConvertermatches a URI path, but these constants come fromPath.of(...).toString(). That makes the.replace(...)calls stop matching on Windows, so fixture normalization leaves the platform-specific path segments behind. Use explicit/literals or normalize the constants once.🤖 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/TestURIConverter.java` around lines 33 - 68, The URI normalization in TestURIConverter is mixing URI path matching with Path.of(...).toString(), which breaks the replace chain on Windows because the separators no longer match. Update the constants used by toString(Object) to use slash-separated URI fragments (or normalize them once up front) so the .replace(...) calls for WORKDIR, EDTDIR, DESIGNERDIR, and the other directory markers consistently strip fixture paths across platforms.
🧹 Nitpick comments (7)
src/test/java/com/github/_1c_syntax/bsl/mdo/children/ObjectFormTest.java (1)
110-148: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract duplicated fixture-discovery logic shared with
ObjectTemplateTest.
discoverObjectFormDataRefs,parseParentRef, and theFormDataFixtureRefrecord are near-identical todiscoverObjectTemplateDataRefs/parseParentRef/TemplateDataFixtureRefinObjectTemplateTest.java(differing only by the fixture sub-directory name and record type). Consider extracting a generic helper (e.g. inFixtures) parameterized by sub-directory name and aBiFunctionconstructing the ref, to avoid the duplicate and keep both test classes in sync.🤖 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/mdo/children/ObjectFormTest.java` around lines 110 - 148, Extract the duplicated fixture-discovery flow in ObjectFormTest by reusing the same helper pattern used in ObjectTemplateTest for discoverObjectTemplateDataRefs, parseParentRef, and the fixture ref record. Move the shared directory-walking and parent-ref parsing logic into a generic utility (for example in Fixtures) that accepts the fixture sub-directory name and a constructor callback for the ref type, then update discoverObjectFormDataRefs and FormDataFixtureRef to delegate to it.src/test/java/com/github/_1c_syntax/bsl/mdo/CalculationRegisterTest.java (1)
90-93: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueUnguarded
getFirst()on recalculations list.
calculationRegister.getRecalculations().getFirst()throwsNoSuchElementExceptionif the list is empty for any of the four CSV rows/fixtures. Given the fixture-driven parameterization, an empty list would produce a confusing failure rather than a clear assertion message.🧪 Suggested guard
- var recalc = calculationRegister.getRecalculations().getFirst(); + assertThat(calculationRegister.getRecalculations()).isNotEmpty(); + var recalc = calculationRegister.getRecalculations().getFirst();🤖 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/mdo/CalculationRegisterTest.java` around lines 90 - 93, The test in CalculationRegisterTest is calling getFirst() on the recalculations list without checking whether it is empty, which can throw a confusing exception for fixture-driven cases. Update the assertion around getRecalculations() to guard against an empty list first, then only inspect the first recalculation when one is present; keep the existing Module::isProtected check tied to the same recalc variable.src/test/java/com/github/_1c_syntax/bsl/mdo/CommonFormTest.java (1)
117-118: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
PathUtils.getBaseName(fixtureFile)directlyfixtureFile.getFileName()is redundant here, and importingPathUtilsis clearer than the fully qualified reference.🤖 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/mdo/CommonFormTest.java` around lines 117 - 118, The fixture name extraction in CommonFormTest is using an unnecessary getFileName() call and a fully qualified PathUtils reference. Update the formRef assignment in the test setup to use PathUtils.getBaseName directly on fixtureFile, and use the imported PathUtils symbol so the intent is clearer and the code is simpler.src/test/java/com/github/_1c_syntax/bsl/mdo/CommonTemplateTest.java (2)
112-135: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueDuplicate filesystem traversal across the two
@TestFactorymethods.
discoverCommonTemplateDataRefs()walksFixtures.FIXTURES_PATHtwice (once per Designer/EDT factory). Low impact given test-only scope, but could be memoized if fixture discovery grows expensive.🤖 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/mdo/CommonTemplateTest.java` around lines 112 - 135, The fixture discovery logic in discoverCommonTemplateDataRefs() is traversing the filesystem separately for both `@TestFactory` paths, which duplicates work. Refactor the CommonTemplateTest setup so the discovered TemplateDataFixtureRef list is computed once and reused by the Designer and EDT factories, for example by memoizing the result in a shared helper or cached field. Keep the discovery behavior unchanged while centralizing the call to discoverCommonTemplateDataRefs().
137-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing
@Nullableon a method that can returnnull.
toGroupRefreturnsnullwhen the ref lacks a dot or the type can't be mapped, but the return type isn't annotated. Per coding guidelines, jspecify null-safety annotations should be used.🏷️ Suggested annotation
- private static String toGroupRef(String mdoRef) { + private static `@Nullable` String toGroupRef(String mdoRef) {As per coding guidelines, "Use
@Nullableand@NonNullannotations from org.jspecify for null safety."🤖 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/mdo/CommonTemplateTest.java` around lines 137 - 144, The toGroupRef method can return null in the invalid-ref and unmapped-type paths, so add the appropriate org.jspecify `@Nullable` annotation to its return type/signature in CommonTemplateTest. Keep the existing logic unchanged, and ensure the annotation matches the project’s null-safety conventions used alongside MDOType.fromValue and toGroupRef.Source: Coding guidelines
src/test/java/com/github/_1c_syntax/bsl/test_utils/Fixtures.java (1)
135-262: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFull classpath scan repeated on every serialization call.
asJson/writecallcreateXstream, which runs a freshClassGraphscan over three packages plus reflectiveClass.forName+ field introspection for every object serialized. GivenFixtureComparisonTest/ChildFixtureComparisonTestinvokeasJsononce per fixture file (potentially dozens/hundreds of dynamic tests), this repeats an expensive classpath scan on every single test instead of once per suite.Consider caching two
XStreaminstances (compact/non-compact) built once and reused across calls.♻️ Suggested caching approach
+ private static final Map<Boolean, XStream> xstreamCache = new ConcurrentHashMap<>(); + public static String asJson(Object obj) { - var xstream = createXstream(obj instanceof CF); + var xstream = xstreamCache.computeIfAbsent(obj instanceof CF, Fixtures::createXstream); xstream.registerConverter(new JavaBeanConverter(xstream.getMapper(), getBeanProvider(), obj.getClass()), -20); return xstream.toXML(obj); }🤖 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/Fixtures.java` around lines 135 - 262, The serialization path in Fixtures.asJson/write rebuilds XStream and reruns the full ClassGraph scan on every call, which is too expensive for repeated fixture tests. Move the XStream setup from createXstream into a cached singleton-style initialization and reuse two prebuilt instances for the compact and non-compact cases, then have asJson select the appropriate cached instance before registering the per-object JavaBeanConverter.src/main/java/com/github/_1c_syntax/bsl/reader/common/context/MDCReaderContext.java (1)
109-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMagic number
99should reference a named constant.The sentinel check
configurationExtensionCompatibilityMode.getVersion() == 99hardcodes an implementation detail ofCompatibilityMode(presumably "unset"/default version) without a named constant, making the intent unclear and fragile to future changes inCompatibilityMode.♻️ Suggested refactor (pending confirmation of the actual constant name)
- } else if (configurationExtensionCompatibilityMode.getVersion() == 99) { + } else if (configurationExtensionCompatibilityMode.getVersion() == CompatibilityMode.NOT_SET_VERSION) {Please confirm whether
CompatibilityModeexposes a named constant for this sentinel value.🤖 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/main/java/com/github/_1c_syntax/bsl/reader/common/context/MDCReaderContext.java` around lines 109 - 113, The sentinel check in MDCReaderContext uses a hardcoded version value, so replace configurationExtensionCompatibilityMode.getVersion() == 99 with a named constant from CompatibilityMode if one exists, or introduce one there and use it here. Update the conditional in the context-writing logic to reference that constant so the intent is clear and the value is centralized.
🤖 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/main/java/com/github/_1c_syntax/bsl/reader/edt/converter/MetaDataObjectConverter.java`:
- Around line 44-67: In MetaDataObjectConverter.unmarshal, the initial
reader.moveDown() is not balanced before returning, leaving the
HierarchicalStreamReader at the wrong depth. Update unmarshal to restore the
reader state by calling reader.moveUp() after the child processing loop and
before return readerContext.build(), keeping the existing readDesignerProperties
and ExtendXStream logic unchanged.
In `@src/test/java/com/github/_1c_syntax/bsl/mdo/AccumulationRegisterTest.java`:
- Around line 72-104: The test in AccumulationRegisterTest contains an
accidental duplicate “AttributeOwner” assertion block; remove the repeated block
so only one copy of the getAllAttributes, getStorageFields, and
getPlainStorageFields assertions remains. Use the existing
AccumulationRegisterTest assertions section and the repeated
getAllAttributes/getStorageFields/getPlainStorageFields calls to identify and
delete the extra copy without changing the intended coverage.
In `@src/test/java/com/github/_1c_syntax/bsl/mdo/CommonFormTest.java`:
- Around line 106-138: The fixture discovery in discoverCommonFormDataRefs can
fail silently and produce an empty test set, so make the failure explicit
instead of returning zero dynamic tests. Add a guard after discovery in the
CommonFormTest flow (or inside discoverCommonFormDataRefs) to assert the refs
list is non-empty, and have toGroupRef fail loudly or log/throw when
MDOType.fromValue cannot map a type instead of returning null. Use the existing
discoverCommonFormDataRefs and toGroupRef helpers to keep the fix localized and
ensure broken fixture layout or mapping issues surface as test failures.
In
`@src/test/java/com/github/_1c_syntax/bsl/test_utils/assertions/Assertions.java`:
- Around line 41-44: The Assertions.assertThat overload currently casts
Collection<? extends MD> to List<MD>, which will fail for non-List collections
such as Set. Update this method to either accept List<MD> explicitly or convert
the incoming collection to a new ArrayList before delegating to
MDCollectionAssert.assertThat, keeping the existing method name so call sites
remain easy to locate.
In
`@src/test/java/com/github/_1c_syntax/bsl/test_utils/assertions/JsonAssert.java`:
- Around line 100-119: The message parsing in JsonAssert.formatMessage skips
valid matches when the target phrase starts at index 0 because the checks use
indexOf(...) > 0. Update the three branches handling "Could not find match for
element", "Missing:", and "Unexpected:" to accept zero-based matches as well, so
the path-prefixed formatting still applies when there is no leading path text.
Keep the fix localized to the JsonAssert formatting logic.
In
`@src/test/java/com/github/_1c_syntax/bsl/test_utils/assertions/ModuleCollectionAssert.java`:
- Around line 146-158: The diagnostic labels in ModuleCollectionAssert’s failure
message are swapped, so the values from missing and extra are described
incorrectly. Update the String.format message in the comparison/fail path to
match the same wording used by MDCollectionAssert.check(), ensuring missing is
labeled as absent from expected and extra is labeled as absent from actual, and
keep the values passed to formatCollection aligned with those labels.
- Around line 90-114: The flatten() helper in ModuleCollectionAssert should
handle null varargs entries the same way as MDCollectionAssert. Update the
collection loop to skip any null collection before calling isEmpty(), so a
missing owner collection does not trigger an NPE. Keep the fix localized to
flatten(Collection<?>...) and preserve the existing ModuleOwner and Module
handling logic.
---
Outside diff comments:
In `@src/test/java/com/github/_1c_syntax/bsl/test_utils/TestURIConverter.java`:
- Around line 33-68: The URI normalization in TestURIConverter is mixing URI
path matching with Path.of(...).toString(), which breaks the replace chain on
Windows because the separators no longer match. Update the constants used by
toString(Object) to use slash-separated URI fragments (or normalize them once up
front) so the .replace(...) calls for WORKDIR, EDTDIR, DESIGNERDIR, and the
other directory markers consistently strip fixture paths across platforms.
---
Nitpick comments:
In
`@src/main/java/com/github/_1c_syntax/bsl/reader/common/context/MDCReaderContext.java`:
- Around line 109-113: The sentinel check in MDCReaderContext uses a hardcoded
version value, so replace configurationExtensionCompatibilityMode.getVersion()
== 99 with a named constant from CompatibilityMode if one exists, or introduce
one there and use it here. Update the conditional in the context-writing logic
to reference that constant so the intent is clear and the value is centralized.
In `@src/test/java/com/github/_1c_syntax/bsl/mdo/CalculationRegisterTest.java`:
- Around line 90-93: The test in CalculationRegisterTest is calling getFirst()
on the recalculations list without checking whether it is empty, which can throw
a confusing exception for fixture-driven cases. Update the assertion around
getRecalculations() to guard against an empty list first, then only inspect the
first recalculation when one is present; keep the existing Module::isProtected
check tied to the same recalc variable.
In `@src/test/java/com/github/_1c_syntax/bsl/mdo/children/ObjectFormTest.java`:
- Around line 110-148: Extract the duplicated fixture-discovery flow in
ObjectFormTest by reusing the same helper pattern used in ObjectTemplateTest for
discoverObjectTemplateDataRefs, parseParentRef, and the fixture ref record. Move
the shared directory-walking and parent-ref parsing logic into a generic utility
(for example in Fixtures) that accepts the fixture sub-directory name and a
constructor callback for the ref type, then update discoverObjectFormDataRefs
and FormDataFixtureRef to delegate to it.
In `@src/test/java/com/github/_1c_syntax/bsl/mdo/CommonFormTest.java`:
- Around line 117-118: The fixture name extraction in CommonFormTest is using an
unnecessary getFileName() call and a fully qualified PathUtils reference. Update
the formRef assignment in the test setup to use PathUtils.getBaseName directly
on fixtureFile, and use the imported PathUtils symbol so the intent is clearer
and the code is simpler.
In `@src/test/java/com/github/_1c_syntax/bsl/mdo/CommonTemplateTest.java`:
- Around line 112-135: The fixture discovery logic in
discoverCommonTemplateDataRefs() is traversing the filesystem separately for
both `@TestFactory` paths, which duplicates work. Refactor the CommonTemplateTest
setup so the discovered TemplateDataFixtureRef list is computed once and reused
by the Designer and EDT factories, for example by memoizing the result in a
shared helper or cached field. Keep the discovery behavior unchanged while
centralizing the call to discoverCommonTemplateDataRefs().
- Around line 137-144: The toGroupRef method can return null in the invalid-ref
and unmapped-type paths, so add the appropriate org.jspecify `@Nullable`
annotation to its return type/signature in CommonTemplateTest. Keep the existing
logic unchanged, and ensure the annotation matches the project’s null-safety
conventions used alongside MDOType.fromValue and toGroupRef.
In `@src/test/java/com/github/_1c_syntax/bsl/test_utils/Fixtures.java`:
- Around line 135-262: The serialization path in Fixtures.asJson/write rebuilds
XStream and reruns the full ClassGraph scan on every call, which is too
expensive for repeated fixture tests. Move the XStream setup from createXstream
into a cached singleton-style initialization and reuse two prebuilt instances
for the compact and non-compact cases, then have asJson select the appropriate
cached instance before registering the per-object JavaBeanConverter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| @SneakyThrows | ||
| @Override | ||
| public Object unmarshal(HierarchicalStreamReader reader, UnmarshallingContext context) { | ||
| reader.moveDown(); | ||
| var nodeClassName = reader.getNodeName(); | ||
| Class<?> realClass = ExtendXStream.getRealClass(reader, nodeClassName); | ||
| if (realClass == null) { | ||
| LOGGER.error("Unexpected type `{}`, path: `{}`", nodeClassName, ExtendXStream.getCurrentPath(reader)); | ||
| throw new IllegalStateException("Unexpected type: " + nodeClassName); | ||
| } | ||
|
|
||
| var readerContext = new MDReaderContext(reader); | ||
|
|
||
| while (reader.hasMoreChildren()) { | ||
| reader.moveDown(); | ||
| var name = reader.getNodeName(); | ||
| if ("Properties".equals(name)) { | ||
| readDesignerProperties(reader, context, readerContext); | ||
| } | ||
| reader.moveUp(); | ||
| } | ||
|
|
||
| return readerContext.build(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the converter and any tests or helper patterns around it.
git ls-files | rg 'MetaDataObjectConverter\.java|ExtendXStream|MDReaderContext|Interface\.mdo|Style\.mdo|ConverterTest|XStream' -n
echo '--- MetaDataObjectConverter.java outline ---'
ast-grep outline src/main/java/com/github/_1c_syntax/bsl/reader/edt/converter/MetaDataObjectConverter.java --view expanded || true
echo '--- Relevant file excerpt ---'
sed -n '1,220p' src/main/java/com/github/_1c_syntax/bsl/reader/edt/converter/MetaDataObjectConverter.java
echo '--- Search for moveDown/moveUp in related code ---'
rg -n "moveDown\(|moveUp\(" src/main/java src/test/java | sed -n '1,220p'Repository: 1c-syntax/mdclasses
Length of output: 16201
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search XStream contracts in the repository or bundled docs if present.
rg -n "A call to moveDown must be balanced with a call to moveUp|HierarchicalStreamReader|moveDown.*moveUp" . | sed -n '1,220p'Repository: 1c-syntax/mdclasses
Length of output: 24883
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect any tests that exercise this converter or neighboring EDT readers.
rg -n "MetaDataObjectConverter|Interface\.mdo|Style\.mdo|reader/edt" src/test/java src/main/java | sed -n '1,240p'Repository: 1c-syntax/mdclasses
Length of output: 476
🌐 Web query:
XStream HierarchicalStreamReader moveDown moveUp contract javadoc
💡 Result:
In the XStream library, the HierarchicalStreamReader interface defines the following contract for the moveDown and moveUp methods: moveDown: This method selects the current child as the current node [1][2][3]. Crucially, the Javadoc specifies that every call to moveDown must be balanced with a corresponding call to moveUp [1][3]. moveUp: This method selects the parent node as the current node [2][3]. These methods are used to navigate hierarchical data structures during deserialization. By adhering to the requirement that moveDown calls must be balanced by moveUp calls, implementers ensure that the reader correctly tracks its position within the node hierarchy [1][4].
Citations:
- 1: https://x-stream.github.io/javadoc/com/thoughtworks/xstream/io/HierarchicalStreamReader.html
- 2: https://x-stream.github.io/1.4.9/javadoc/com/thoughtworks/xstream/io/HierarchicalStreamReader.html
- 3: https://apidia.net/mvn/com.thoughtworks.xstream/xstream/1.4.20/com.thoughtworks.xstream.io.HierarchicalStreamReader.html
- 4: https://x-stream.github.io/javadoc/com/thoughtworks/xstream/io/ReaderWrapper.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- EDT MetaDataObjectConverter around unmarshal ---'
sed -n '1,180p' src/main/java/com/github/_1c_syntax/bsl/reader/edt/converter/MetaDataObjectConverter.java
echo '--- designer MetaDataObjectConverter around unmarshal ---'
sed -n '1,170p' src/main/java/com/github/_1c_syntax/bsl/reader/designer/converter/MetaDataObjectConverter.java
echo '--- MDReaderContext constructor and build ---'
sed -n '1,220p' src/main/java/com/github/_1c_syntax/bsl/reader/common/context/MDReaderContext.java
echo '--- EDT Reader unmarshal entrypoint ---'
sed -n '220,290p' src/main/java/com/github/_1c_syntax/bsl/reader/edt/EDTReader.java
echo '--- EDT converter unmarshal dispatcher ---'
sed -n '1,180p' src/main/java/com/github/_1c_syntax/bsl/reader/edt/converter/Unmarshaller.javaRepository: 1c-syntax/mdclasses
Length of output: 25319
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- ExtendXStream.readValue and related helpers ---'
sed -n '140,240p' src/main/java/com/github/_1c_syntax/bsl/reader/common/xstream/ExtendXStream.java
echo '--- ReadConverter interface / base helpers ---'
sed -n '1,220p' src/main/java/com/github/_1c_syntax/bsl/reader/common/xstream/ReadConverter.java
echo '--- EDT designer-related converters for comparison ---'
sed -n '1,140p' src/main/java/com/github/_1c_syntax/bsl/reader/edt/converter/MdoReferenceConverter.java
sed -n '1,120p' src/main/java/com/github/_1c_syntax/bsl/reader/edt/converter/ExternalSourceConverter.javaRepository: 1c-syntax/mdclasses
Length of output: 8578
Balance the initial moveDown() before returning. The method descends into the child node but never calls moveUp() before return readerContext.build();, leaving the reader at the wrong depth and risking later parsing errors.
🤖 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/main/java/com/github/_1c_syntax/bsl/reader/edt/converter/MetaDataObjectConverter.java`
around lines 44 - 67, In MetaDataObjectConverter.unmarshal, the initial
reader.moveDown() is not balanced before returning, leaving the
HierarchicalStreamReader at the wrong depth. Update unmarshal to restore the
reader state by calling reader.moveUp() after the child processing loop and
before return readerContext.build(), keeping the existing readDesignerProperties
and ExtendXStream logic unchanged.
| private static List<FormDataFixtureRef> discoverCommonFormDataRefs() { | ||
| List<FormDataFixtureRef> refs = new ArrayList<>(); | ||
| try (var packsStream = Files.list(Fixtures.FIXTURES_PATH)) { | ||
| packsStream.filter(Files::isDirectory) | ||
| .forEach(packDir -> { | ||
| var packName = packDir.getFileName().toString(); | ||
| var formdataDir = packDir.resolve("formdata"); | ||
| if (!Files.isDirectory(formdataDir)) return; | ||
| try (var filesStream = Files.list(formdataDir)) { | ||
| filesStream.filter(path -> path.toString().endsWith(".json")) | ||
| .forEach(fixtureFile -> { | ||
| var formRef = org.apache.commons.io.file.PathUtils | ||
| .getBaseName(fixtureFile.getFileName()); | ||
| if (!formRef.startsWith("CommonForm.")) return; | ||
| var groupRef = toGroupRef(formRef); | ||
| if (groupRef != null) { | ||
| refs.add(new FormDataFixtureRef(packName, formRef, groupRef, fixtureFile)); | ||
| } | ||
| }); | ||
| } catch (IOException e) { throw new RuntimeException(e); } | ||
| }); | ||
| } catch (IOException e) { throw new RuntimeException(e); } | ||
| return refs; | ||
| } | ||
|
|
||
| private static String toGroupRef(String mdoRef) { | ||
| var dotIndex = mdoRef.indexOf('.'); | ||
| if (dotIndex < 0) return null; | ||
| var typeStr = mdoRef.substring(0, dotIndex); | ||
| var name = mdoRef.substring(dotIndex + 1); | ||
| var mdoTypeOpt = MDOType.fromValue(typeStr); | ||
| return mdoTypeOpt.map(mdoType -> mdoType.groupName() + "." + name).orElse(null); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Silent zero-test outcome if fixture discovery/mapping fails.
If Fixtures.FIXTURES_PATH/formdata layout changes or toGroupRef fails to map a type (returns null at Line 137 without any error/log), discoverCommonFormDataRefs() can silently return an empty list. compareFormDataDesigner/compareFormDataEDT would then generate zero dynamic tests — a vacuous pass that hides fixture coverage regressions.
Consider asserting the discovered list is non-empty (or failing loudly when toGroupRef can't map a type) so a broken discovery path surfaces as a test failure rather than silent success.
🧪 Suggested guard
private static List<FormDataFixtureRef> discoverCommonFormDataRefs() {
List<FormDataFixtureRef> refs = new ArrayList<>();
try (var packsStream = Files.list(Fixtures.FIXTURES_PATH)) {
...
} catch (IOException e) { throw new RuntimeException(e); }
+ if (refs.isEmpty()) {
+ throw new IllegalStateException("No CommonForm formdata fixtures discovered under " + Fixtures.FIXTURES_PATH);
+ }
return refs;
}📝 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.
| private static List<FormDataFixtureRef> discoverCommonFormDataRefs() { | |
| List<FormDataFixtureRef> refs = new ArrayList<>(); | |
| try (var packsStream = Files.list(Fixtures.FIXTURES_PATH)) { | |
| packsStream.filter(Files::isDirectory) | |
| .forEach(packDir -> { | |
| var packName = packDir.getFileName().toString(); | |
| var formdataDir = packDir.resolve("formdata"); | |
| if (!Files.isDirectory(formdataDir)) return; | |
| try (var filesStream = Files.list(formdataDir)) { | |
| filesStream.filter(path -> path.toString().endsWith(".json")) | |
| .forEach(fixtureFile -> { | |
| var formRef = org.apache.commons.io.file.PathUtils | |
| .getBaseName(fixtureFile.getFileName()); | |
| if (!formRef.startsWith("CommonForm.")) return; | |
| var groupRef = toGroupRef(formRef); | |
| if (groupRef != null) { | |
| refs.add(new FormDataFixtureRef(packName, formRef, groupRef, fixtureFile)); | |
| } | |
| }); | |
| } catch (IOException e) { throw new RuntimeException(e); } | |
| }); | |
| } catch (IOException e) { throw new RuntimeException(e); } | |
| return refs; | |
| } | |
| private static String toGroupRef(String mdoRef) { | |
| var dotIndex = mdoRef.indexOf('.'); | |
| if (dotIndex < 0) return null; | |
| var typeStr = mdoRef.substring(0, dotIndex); | |
| var name = mdoRef.substring(dotIndex + 1); | |
| var mdoTypeOpt = MDOType.fromValue(typeStr); | |
| return mdoTypeOpt.map(mdoType -> mdoType.groupName() + "." + name).orElse(null); | |
| } | |
| private static List<FormDataFixtureRef> discoverCommonFormDataRefs() { | |
| List<FormDataFixtureRef> refs = new ArrayList<>(); | |
| try (var packsStream = Files.list(Fixtures.FIXTURES_PATH)) { | |
| packsStream.filter(Files::isDirectory) | |
| .forEach(packDir -> { | |
| var packName = packDir.getFileName().toString(); | |
| var formdataDir = packDir.resolve("formdata"); | |
| if (!Files.isDirectory(formdataDir)) return; | |
| try (var filesStream = Files.list(formdataDir)) { | |
| filesStream.filter(path -> path.toString().endsWith(".json")) | |
| .forEach(fixtureFile -> { | |
| var formRef = org.apache.commons.io.file.PathUtils | |
| .getBaseName(fixtureFile.getFileName()); | |
| if (!formRef.startsWith("CommonForm.")) return; | |
| var groupRef = toGroupRef(formRef); | |
| if (groupRef != null) { | |
| refs.add(new FormDataFixtureRef(packName, formRef, groupRef, fixtureFile)); | |
| } | |
| }); | |
| } catch (IOException e) { throw new RuntimeException(e); } | |
| }); | |
| } catch (IOException e) { throw new RuntimeException(e); } | |
| if (refs.isEmpty()) { | |
| throw new IllegalStateException("No CommonForm formdata fixtures discovered under " + Fixtures.FIXTURES_PATH); | |
| } | |
| return refs; | |
| } | |
| private static String toGroupRef(String mdoRef) { | |
| var dotIndex = mdoRef.indexOf('.'); | |
| if (dotIndex < 0) return null; | |
| var typeStr = mdoRef.substring(0, dotIndex); | |
| var name = mdoRef.substring(dotIndex + 1); | |
| var mdoTypeOpt = MDOType.fromValue(typeStr); | |
| return mdoTypeOpt.map(mdoType -> mdoType.groupName() + "." + name).orElse(null); | |
| } |
🤖 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/mdo/CommonFormTest.java` around lines
106 - 138, The fixture discovery in discoverCommonFormDataRefs can fail silently
and produce an empty test set, so make the failure explicit instead of returning
zero dynamic tests. Add a guard after discovery in the CommonFormTest flow (or
inside discoverCommonFormDataRefs) to assert the refs list is non-empty, and
have toGroupRef fail loudly or log/throw when MDOType.fromValue cannot map a
type instead of returning null. Use the existing discoverCommonFormDataRefs and
toGroupRef helpers to keep the fix localized and ensure broken fixture layout or
mapping issues surface as test failures.
| @SuppressWarnings("unchecked") | ||
| public static MDCollectionAssert assertThat(Collection<? extends MD> actual, boolean ignored) { | ||
| return MDCollectionAssert.assertThat((List<MD>) actual, ignored); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and related assertion types.
git ls-files 'src/test/java/com/github/_1c_syntax/bsl/test_utils/assertions/**' | sed -n '1,200p'
echo
echo '--- Assertions.java ---'
sed -n '1,140p' src/test/java/com/github/_1c_syntax/bsl/test_utils/assertions/Assertions.java
echo
echo '--- Search MDCollectionAssert ---'
rg -n "class MDCollectionAssert|static .*assertThat\(.*List<MD>|assertThat\(Collection|assertThat\(List" src/test/java/com/github/_1c_syntax/bsl/test_utils/assertions -SRepository: 1c-syntax/mdclasses
Length of output: 3413
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- MDCollectionAssert.java ---'
sed -n '1,220p' src/test/java/com/github/_1c_syntax/bsl/test_utils/assertions/MDCollectionAssert.java
echo
echo '--- Call sites of Assertions.assertThat(..., boolean) for MD collections ---'
rg -n "assertThat\([^)]*MD|assertThat\([^)]*Collection<\? extends MD>|Assertions\.assertThat\(" src/test/java src/main/java -SRepository: 1c-syntax/mdclasses
Length of output: 28612
Avoid casting Collection<? extends MD> to List<MD> here. A non-List collection like a Set will fail with ClassCastException; wrap it with new ArrayList<>(actual) or narrow the overload to List<MD>.
🤖 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/assertions/Assertions.java`
around lines 41 - 44, The Assertions.assertThat overload currently casts
Collection<? extends MD> to List<MD>, which will fail for non-List collections
such as Set. Update this method to either accept List<MD> explicitly or convert
the incoming collection to a new ArrayList before delegating to
MDCollectionAssert.assertThat, keeping the existing method name so call sites
remain easy to locate.
| // "path Could not find match for element {...}" | ||
| int noMatchIdx = trimmed.indexOf("Could not find match for element"); | ||
| if (noMatchIdx > 0) { | ||
| String path = trimmed.substring(0, noMatchIdx).trim(); | ||
| return " Cannot match element at: " + path; | ||
| } | ||
|
|
||
| // "path Missing: ..." | ||
| int missingIdx = trimmed.indexOf("Missing:"); | ||
| if (missingIdx > 0) { | ||
| String path = trimmed.substring(0, missingIdx).trim(); | ||
| return " Missing: " + path + "\n " + trimmed.substring(missingIdx); | ||
| } | ||
|
|
||
| // "path Unexpected: ..." | ||
| int unexpectedIdx = trimmed.indexOf("Unexpected:"); | ||
| if (unexpectedIdx > 0) { | ||
| String path = trimmed.substring(0, unexpectedIdx).trim(); | ||
| return " Unexpected: " + path + "\n " + trimmed.substring(unexpectedIdx); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Boundary check > 0 misses matches at the start of the message.
Each branch uses indexOf(...) > 0 to decide whether to format the path-prefixed message. If the target phrase (e.g., "Could not find match for element") occurs at index 0 (no path prefix), the condition is false and the branch is skipped, falling through to worse formatting (the huge-message truncation or raw dump). Use >= 0 to also handle a zero-length prefix.
🐛 Suggested fix
int noMatchIdx = trimmed.indexOf("Could not find match for element");
- if (noMatchIdx > 0) {
+ if (noMatchIdx >= 0) {
String path = trimmed.substring(0, noMatchIdx).trim();
return " Cannot match element at: " + path;
}
// "path Missing: ..."
int missingIdx = trimmed.indexOf("Missing:");
- if (missingIdx > 0) {
+ if (missingIdx >= 0) {
String path = trimmed.substring(0, missingIdx).trim();
return " Missing: " + path + "\n " + trimmed.substring(missingIdx);
}
// "path Unexpected: ..."
int unexpectedIdx = trimmed.indexOf("Unexpected:");
- if (unexpectedIdx > 0) {
+ if (unexpectedIdx >= 0) {
String path = trimmed.substring(0, unexpectedIdx).trim();
return " Unexpected: " + path + "\n " + trimmed.substring(unexpectedIdx);
}📝 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.
| // "path Could not find match for element {...}" | |
| int noMatchIdx = trimmed.indexOf("Could not find match for element"); | |
| if (noMatchIdx > 0) { | |
| String path = trimmed.substring(0, noMatchIdx).trim(); | |
| return " Cannot match element at: " + path; | |
| } | |
| // "path Missing: ..." | |
| int missingIdx = trimmed.indexOf("Missing:"); | |
| if (missingIdx > 0) { | |
| String path = trimmed.substring(0, missingIdx).trim(); | |
| return " Missing: " + path + "\n " + trimmed.substring(missingIdx); | |
| } | |
| // "path Unexpected: ..." | |
| int unexpectedIdx = trimmed.indexOf("Unexpected:"); | |
| if (unexpectedIdx > 0) { | |
| String path = trimmed.substring(0, unexpectedIdx).trim(); | |
| return " Unexpected: " + path + "\n " + trimmed.substring(unexpectedIdx); | |
| } | |
| // "path Could not find match for element {...}" | |
| int noMatchIdx = trimmed.indexOf("Could not find match for element"); | |
| if (noMatchIdx >= 0) { | |
| String path = trimmed.substring(0, noMatchIdx).trim(); | |
| return " Cannot match element at: " + path; | |
| } | |
| // "path Missing: ..." | |
| int missingIdx = trimmed.indexOf("Missing:"); | |
| if (missingIdx >= 0) { | |
| String path = trimmed.substring(0, missingIdx).trim(); | |
| return " Missing: " + path + "\n " + trimmed.substring(missingIdx); | |
| } | |
| // "path Unexpected: ..." | |
| int unexpectedIdx = trimmed.indexOf("Unexpected:"); | |
| if (unexpectedIdx >= 0) { | |
| String path = trimmed.substring(0, unexpectedIdx).trim(); | |
| return " Unexpected: " + path + "\n " + trimmed.substring(unexpectedIdx); | |
| } |
🤖 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/assertions/JsonAssert.java`
around lines 100 - 119, The message parsing in JsonAssert.formatMessage skips
valid matches when the target phrase starts at index 0 because the checks use
indexOf(...) > 0. Update the three branches handling "Could not find match for
element", "Missing:", and "Unexpected:" to accept zero-based matches as well, so
the path-prefixed formatting still applies when there is no leading path text.
Keep the fix localized to the JsonAssert formatting logic.
| private List<Module> flatten(Collection<?>... collections) { | ||
| List<Module> result = new ArrayList<>(); | ||
| for (var collection : collections) { | ||
| if (collection.isEmpty()) { | ||
| continue; | ||
| } | ||
| var first = collection.iterator().next(); | ||
| if (first instanceof ModuleOwner) { | ||
| // коллекция содержит ModuleOwner (Form, Command, и т.д.) — извлекаем модули | ||
| for (var item : collection) { | ||
| if (item instanceof ModuleOwner owner) { | ||
| result.addAll(owner.getAllModules()); | ||
| } | ||
| } | ||
| } else if (first instanceof Module) { | ||
| // коллекция содержит Module — просто добавляем | ||
| for (var item : collection) { | ||
| if (item instanceof Module module) { | ||
| result.add(module); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Missing null-guard in flatten() — inconsistent with MDCollectionAssert.
MDCollectionAssert.flatten guards with collection == null || collection.isEmpty(), but this method only checks collection.isEmpty(). A null element in the varargs (e.g. an owner collection that wasn't initialized) will NPE here instead of being skipped.
🛡️ Proposed fix
for (var collection : collections) {
- if (collection.isEmpty()) {
+ if (collection == null || collection.isEmpty()) {
continue;
}📝 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.
| private List<Module> flatten(Collection<?>... collections) { | |
| List<Module> result = new ArrayList<>(); | |
| for (var collection : collections) { | |
| if (collection.isEmpty()) { | |
| continue; | |
| } | |
| var first = collection.iterator().next(); | |
| if (first instanceof ModuleOwner) { | |
| // коллекция содержит ModuleOwner (Form, Command, и т.д.) — извлекаем модули | |
| for (var item : collection) { | |
| if (item instanceof ModuleOwner owner) { | |
| result.addAll(owner.getAllModules()); | |
| } | |
| } | |
| } else if (first instanceof Module) { | |
| // коллекция содержит Module — просто добавляем | |
| for (var item : collection) { | |
| if (item instanceof Module module) { | |
| result.add(module); | |
| } | |
| } | |
| } | |
| } | |
| return result; | |
| } | |
| private List<Module> flatten(Collection<?>... collections) { | |
| List<Module> result = new ArrayList<>(); | |
| for (var collection : collections) { | |
| if (collection == null || collection.isEmpty()) { | |
| continue; | |
| } | |
| var first = collection.iterator().next(); | |
| if (first instanceof ModuleOwner) { | |
| // коллекция содержит ModuleOwner (Form, Command, и т.д.) — извлекаем модули | |
| for (var item : collection) { | |
| if (item instanceof ModuleOwner owner) { | |
| result.addAll(owner.getAllModules()); | |
| } | |
| } | |
| } else if (first instanceof Module) { | |
| // коллекция содержит Module — просто добавляем | |
| for (var item : collection) { | |
| if (item instanceof Module module) { | |
| result.add(module); | |
| } | |
| } | |
| } | |
| } | |
| return result; | |
| } |
🤖 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/assertions/ModuleCollectionAssert.java`
around lines 90 - 114, The flatten() helper in ModuleCollectionAssert should
handle null varargs entries the same way as MDCollectionAssert. Update the
collection loop to skip any null collection before calling isEmpty(), so a
missing owner collection does not trigger an NPE. Keep the fix localized to
flatten(Collection<?>...) and preserve the existing ModuleOwner and Module
handling logic.
| String msg = String.format( | ||
| """ | ||
| Ожидаемые и фактические модули различаются: | ||
| ожидаются (expected): | ||
| %s | ||
|
|
||
| отсутствуют в expected (extra): | ||
| %s | ||
| """, | ||
| formatCollection(missing), | ||
| formatCollection(extra) | ||
| ); | ||
| failWithMessage(msg); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Diagnostic message labels are swapped — "expected"/"extra" don't match the values printed.
The message labels the first %s (which is missing) as "ожидаются (expected)" and the second %s (which is extra) as "отсутствуют в expected (extra)". Compare with MDCollectionAssert.check(), which correctly labels missing as "отсутствуют в expected (missing)" and extra as "отсутствуют в actual (extra)". As written, a test failure here will show confusing/incorrect diagnostics.
🐛 Proposed fix
String msg = String.format(
"""
Ожидаемые и фактические модули различаются:
- ожидаются (expected):
+ отсутствуют в expected (missing):
%s
- отсутствуют в expected (extra):
+ отсутствуют в actual (extra):
%s
""",
formatCollection(missing),
formatCollection(extra)
);📝 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.
| String msg = String.format( | |
| """ | |
| Ожидаемые и фактические модули различаются: | |
| ожидаются (expected): | |
| %s | |
| отсутствуют в expected (extra): | |
| %s | |
| """, | |
| formatCollection(missing), | |
| formatCollection(extra) | |
| ); | |
| failWithMessage(msg); | |
| String msg = String.format( | |
| """ | |
| Ожидаемые и фактические модули различаются: | |
| отсутствуют в expected (missing): | |
| %s | |
| отсутствуют в actual (extra): | |
| %s | |
| """, | |
| formatCollection(missing), | |
| formatCollection(extra) | |
| ); | |
| failWithMessage(msg); |
🤖 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/assertions/ModuleCollectionAssert.java`
around lines 146 - 158, The diagnostic labels in ModuleCollectionAssert’s
failure message are swapped, so the values from missing and extra are described
incorrectly. Update the String.format message in the comparison/fail path to
match the same wording used by MDCollectionAssert.check(), ensuring missing is
labeled as absent from expected and extra is labeled as absent from actual, and
keep the values passed to formatCollection aligned with those labels.
|
1 participant
Описание
Связанные задачи
Closes
Чеклист
Общие
gradlew precommit)Дополнительно
Summary by CodeRabbit
New Features
Bug Fixes