Skip to content
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

Investigate a way to manually define test specs (specs.json) #616

Closed
rklfss opened this issue Oct 10, 2017 · 7 comments
Closed

Investigate a way to manually define test specs (specs.json) #616

rklfss opened this issue Oct 10, 2017 · 7 comments

Comments

@rklfss
Copy link
Contributor

rklfss commented Oct 10, 2017

I'm wondering what the tests comparing the specs.json are good for and if there is any case of failing?

Reason: the specs.json files are generated by the same well-functioning or malfunctioning program that generates the output to compare with (=my locally generated typedoc build).

gruntfile.js

var out = Path.join(base, directory, 'specs.json');
var result = app.convert(src);
var data = JSON.stringify(result.toObject(), null, '  ');
data = data.split(TypeDoc.normalizePath(base)).join('%BASE%');
FS.writeFileSync(out, data);

test/converter.ts

const specs = JSON.parse(FS.readFileSync(Path.join(path, 'specs.json')).toString());
let data = JSON.stringify(result.toObject(), null, '  ');
data = data.split(normalizePath(base)).join('%BASE%');

compareReflections(JSON.parse(data), specs);

The tests always succeed since the specs.json is just the written-to-a-file output that is generated a second time for comparison although the output could be wrong regarding its content.

Example:

module TestModule
{
    /**
     * TestClass class comment.
     */
    export class TestClass
    {
        /**
         * This is a property.
         */
        public property = 'property';
        
        /**
         * This is a method.
         */
        public method(): void
        {

        }
    }

    /**
     * TestClass module comment.
     */
    export module TestClass
    {
        /**
         * This is an exported variable.
         */
        export const exportedVar = 'exported';

        /**
         * This is an internal variable.
         */
        const internalVar = 'internal';

        /**
         * This is an internal function.
         */
        function internalFunction(): void
        {

        }
    }
}
...
                {
                  "id": 8,
                  "name": "internalVar",
                  "kind": 1024,
                  "kindString": "Property",
                  "flags": {
                    "isStatic": true,
                    "isExported": true
                  },
                  "comment": {
                    "shortText": "This is an internal variable."
                  },
                  "sources": [
                    {
                      "fileName": "nested-modules.ts",
                      "line": 35,
                      "character": 25
                    }
                  ],
                  "type": {
                    "type": "stringLiteral",
                    "value": "internal"
                  },
                  "defaultValue": "\"internal\""
                },

...

The isExported flag of "internalVar" is true although it should not.

I would like us to investigate a way to manuelly define specs saying "isExported flag of internalVar must not be true" or things like that.

@aciccarello
Copy link
Collaborator

That sounds like a great idea! The tests could use a lot of work. I also open #542 to allow more flexibility when testing. Since I didn't write the lib, the lack of effective tests makes it especially hard to review PRs. I'd love to see this implemented.

@aciccarello
Copy link
Collaborator

I just want to note that I'm also seeing that tests pass when they shouldn't. You can tell that the test shouldn't pass because the specs.json file is changed.

@kayahr
Copy link

kayahr commented Jun 6, 2018

What about merging the small PR #762 as a quick fix for this issue? This PR disables the re-building of the specs.json files during the build so the tests actually do something useful (Comparing the new results with the previously committed results). In case something changes and these changes are correct then you can run the grunt task update-specs to update the files and commit them.

I just tested this PR and it works fine. I sabotaged typedoc by changing Type.isTypeListEqual to always return false and then did a full rebuild and the unit tests successfully found the differences caused by this change. This was not the case before because the build silently replaced the JSON files and then ran the tests against the already updated files.

And don't be alarmed by the unit test failures reported for this PR. They are there because the unit tests now actually do something. So these errors are normal and can be fixed by committing the updated spec files once.

@aciccarello
Copy link
Collaborator

Yep, I'm working on updating the specs now. I do think that there are some bugs to fix though so I'm held up on that. If I merged #762 now, I'm concerned any PRs would try to change unrelated specs.

Thanks for testing it out!

@aciccarello
Copy link
Collaborator

While investigating this I've noticed that the tests don't run consistently if you run a converter test individually. More investigation is needed to understand that issue.

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Aug 12, 2018

I see #762 has been merged. Is this issue now resolved or does more work need to be done before tests can be relied on?

@aciccarello
Copy link
Collaborator

Yeah, I think it can be. 👍

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

No branches or pull requests

4 participants