-
Notifications
You must be signed in to change notification settings - Fork 38
Specobject bugfixes #169
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
Specobject bugfixes #169
Changes from all commits
fbde45c
8fbb02b
560b866
556c1cd
fd5c585
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -199,7 +199,7 @@ private void writeCoveredIds(final List<SpecificationItemId> coveredIds) | |
| for (final SpecificationItemId coveredId : coveredIds) | ||
| { | ||
| this.writer.writeStartElement("provcov"); | ||
| writeElement("linksto", coveredId.getName()); | ||
| writeElement("linksto", coveredId.getArtifactType() + ":" + coveredId.getName()); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is correct since it will break in cases where people use existing ReqM documents without our proprietary
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This only affects the export. The importer is unchanged. If we don't modify the exporter like this, the importer will use |
||
| writeElement("dstversion", coveredId.getRevision()); | ||
| this.writer.writeEndElement(); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -168,7 +168,7 @@ public void setTitle(final String title) | |
| @Override | ||
| public void setLocation(final String path, final int line) | ||
| { | ||
| this.location = Location.create(path, line); | ||
| this.setLocation(Location.create(path, line)); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Ooops, that kind of design slip should not happen. whistles slightly embarrassed |
||
| } | ||
|
|
||
| @Override | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -71,8 +71,8 @@ private void handleStartElement(final TreeElement elem) | |
| private void handleEndElement() | ||
| { | ||
| this.listener.setId(this.idBuilder.build()); | ||
| this.listener.endSpecificationItem(); | ||
| this.listener.setLocation(this.locationBuilder.build()); | ||
| this.listener.endSpecificationItem(); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, that's a nasty one. Good job uncovering that! |
||
| this.idBuilder = null; | ||
| this.locationBuilder = null; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -100,7 +100,7 @@ public void testExportSpecObjectWithOptionalElements() throws IOException, XMLSt | |
| + " </needscoverage>\n" // | ||
| + " <providescoverage>\n" // | ||
| + " <provcov>\n" // | ||
| + " <linksto>covered</linksto>\n" // | ||
| + " <linksto>feat:covered</linksto>\n" // | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is what I meant earlier: existing documents will break. There must be a more robust solution that at least allows you to import a specobject and get the same back in export. In absence of a better idea I suggest an exporter configuration switch. The current solution fixes one thing by breaking another. |
||
| + " <dstversion>1</dstversion>\n" // | ||
| + " </provcov>\n" // | ||
| + " </providescoverage>\n" // | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,7 @@ | |
| */ | ||
|
|
||
| import java.io.BufferedReader; | ||
| import java.io.StringReader; | ||
| import java.nio.file.Path; | ||
|
|
||
| public class StreamInput implements InputFile | ||
|
|
@@ -36,6 +37,21 @@ private StreamInput(final Path path, final BufferedReader reader) | |
| this.reader = reader; | ||
| } | ||
|
|
||
| /** | ||
| * Create an {@link InputFile} for a given file content. This is useful for | ||
| * tests to avoid using real files. | ||
| * | ||
| * @param path | ||
| * a dummy path. | ||
| * @param content | ||
| * the file content. | ||
| * @return an {@link InputFile}. | ||
| */ | ||
| public static InputFile forContent(final Path path, final String content) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since it is under |
||
| { | ||
| return forReader(path, new BufferedReader(new StringReader(content))); | ||
| } | ||
|
|
||
| /** | ||
| * Create an {@link InputFile} for a {@link BufferedReader}. This is useful | ||
| * for tests to avoid using real files. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,115 @@ | ||
| package org.itsallcode.openfasttrace.importer.specobject; | ||
|
|
||
| /*- | ||
| * #%L | ||
| * OpenFastTrace | ||
| * %% | ||
| * Copyright (C) 2016 - 2018 itsallcode.org | ||
| * %% | ||
| * This program is free software: you can redistribute it and/or modify | ||
| * it under the terms of the GNU General Public License as | ||
| * published by the Free Software Foundation, either version 3 of the | ||
| * License, or (at your option) any later version. | ||
| * | ||
| * This program is distributed in the hope that it will be useful, | ||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
| * GNU General Public License for more details. | ||
| * | ||
| * You should have received a copy of the GNU General Public | ||
| * License along with this program. If not, see | ||
| * <http://www.gnu.org/licenses/gpl-3.0.html>. | ||
| * #L% | ||
| */ | ||
|
|
||
| import static org.hamcrest.MatcherAssert.assertThat; | ||
| import static org.hamcrest.Matchers.hasSize; | ||
|
|
||
| import java.nio.file.Paths; | ||
| import java.util.List; | ||
|
|
||
| import org.itsallcode.openfasttrace.core.*; | ||
| import org.itsallcode.openfasttrace.importer.Importer; | ||
| import org.itsallcode.openfasttrace.importer.SpecificationListBuilder; | ||
| import org.itsallcode.openfasttrace.importer.input.InputFile; | ||
| import org.itsallcode.openfasttrace.importer.input.StreamInput; | ||
| import org.junit.Test; | ||
|
|
||
| import com.github.hamstercommunity.matcher.auto.AutoMatcher; | ||
|
|
||
| public class TestSpecobjectImportExport | ||
| { | ||
| @Test | ||
| public void testTraceContent() | ||
| { | ||
| final String content = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" // | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Java Designers should finally make heredocs part of the language. :-) |
||
| + "<specdocument>\n" // | ||
| + " <specobjects doctype=\"impl\">\n" // | ||
| + " <specobject>\n" // | ||
| + " <id>exampleB-3454416016</id>\n" | ||
| + " <status>approved</status>\n" // | ||
| + " <version>0</version>\n" // | ||
| + " <sourcefile>source.java</sourcefile>\n" // | ||
| + " <sourceline>1</sourceline>\n" // | ||
| + " <providescoverage>\n" // | ||
| + " <provcov>\n" // | ||
| + " <linksto>dsn:exampleB</linksto>\n" | ||
| + " <dstversion>1</dstversion>\n" | ||
| + " </provcov>\n" // | ||
| + " </providescoverage>\n" // | ||
| + " </specobject>\n" // | ||
| + " </specobjects>\n" // | ||
| + " <specobjects doctype=\"dsn\">\n" // | ||
| + " <specobject>\n" // | ||
| + " <id>exampleB</id>\n" // | ||
| + " <status>approved</status>\n" // | ||
| + " <version>1</version>\n" | ||
| + " <sourcefile>spec.md</sourcefile>\n" | ||
| + " <sourceline>2</sourceline>\n" | ||
| + " <description>Example requirement</description>\n" | ||
| + " <needscoverage>\n" // | ||
| + " <needsobj>utest</needsobj>\n" | ||
| + " <needsobj>impl</needsobj>\n" // | ||
| + " </needscoverage>\n" // | ||
| + " </specobject>\n" // | ||
| + " </specobjects>\n" // | ||
| + "</specdocument>"; | ||
|
|
||
| final Trace trace = trace(content); | ||
| assertThat(trace.getItems(), hasSize(2)); | ||
| assertThat(trace.getDefectItems(), hasSize(2)); | ||
|
|
||
| final LinkedSpecificationItem tag = trace.getItems().get(0); | ||
| final LinkedSpecificationItem req = trace.getItems().get(1); | ||
|
|
||
| final SpecificationItem expectedTag = new SpecificationItem.Builder() | ||
| .id("impl", "exampleB-3454416016", 0).location("source.java", 1) | ||
| .status(ItemStatus.APPROVED).addCoveredId("dsn", "exampleB", 1).build(); | ||
| final SpecificationItem expectedReq = new SpecificationItem.Builder() | ||
| .id("dsn", "exampleB", 1).location("spec.md", 2).description("Example requirement") | ||
| .addNeedsArtifactType("utest").addNeedsArtifactType("impl").build(); | ||
|
|
||
| assertThat(tag.getItem(), AutoMatcher.equalTo(expectedTag)); | ||
| assertThat(req.getItem(), AutoMatcher.equalTo(expectedReq)); | ||
| } | ||
|
|
||
| private Trace trace(final String content) | ||
| { | ||
| return trace(parse(content)); | ||
| } | ||
|
|
||
| private Trace trace(final List<SpecificationItem> items) | ||
| { | ||
| final List<LinkedSpecificationItem> linkedItems = new Linker(items).link(); | ||
| return new Tracer().trace(linkedItems); | ||
| } | ||
|
|
||
| private List<SpecificationItem> parse(final String content) | ||
| { | ||
| final SpecificationListBuilder listener = SpecificationListBuilder.create(); | ||
| final InputFile input = StreamInput.forContent(Paths.get("dummy.xml"), content); | ||
| final Importer importer = new SpecobjectImporterFactory().createImporter(input, listener); | ||
| importer.runImport(); | ||
| return listener.build(); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might potentially spam the log.
The warning is okay in cases where the error is rare though.
Longterm question: I thought about formalizing importer error collection. Instead of a regular text log a separate data structure that collects meta data about defect or spurious specification items? Opinions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding error collection: 👍