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

Fix: Test Suite #166

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

Ayplow
Copy link

@Ayplow Ayplow commented Aug 9, 2019

There were a few issues causing the test suite to fail. This PR cleans them up so that work can be done on new features

  • Incorrect Types
    • RSTPreviewConfiguration.lineBreaks: unknown to boolean
    • CodeLineElement.element: HTMLElement to Element
      • Also rewrote the assignment to avoid type assertions
  • Invalid logger object in test/extension.test.ts
    • Changes type to { [P in keyof Logger]: Logger[P] } as Logger, enforcing the methods implementation
  • conf.py referenced the non-existent _static folder, so that was removed
  • sphinx updates have caused the output of the build to change, so that was updated
    • This test could run sphinx itself to check the content of the webview against
  • The compile function could be run before python.setup, leaving it without an interpreter to use.
    • As the tests would never call setup, this meant it was impossible for the compile function to succeed. This was also a problem in use, causing a race condition between the language features and a preview being opened, so I changed it in the implementation

From what I saw earlier, the azure tests will probably fail due to some issue with tsc. I'm not able to troubleshoot this as I dont have any linux setup at the moment. Are you able to look at it?

Typescript should've been failing since vscode-restructuredtext@74cb99b because of this? Have the errors been suppressed somehow?
This will also create a notice if the logger class is ever updated
test-resources\sphinx> sphinx-build . _build\html
test-resources\sphinx> cp _build\html\index.html ..
@oharboe
Copy link

oharboe commented Oct 14, 2019

@lextm I'm wondering if there's a reason why there's no comment on this PR. Maintaining tests seems like a most commendable effort of cleanup and maintenance that should be especially welcome in open source projects.

@lextm
Copy link
Member

lextm commented Oct 14, 2019

@oharboe the horrible part of running an open source project is to avoid taking everything.

Note that the original preview code was take directly from VSCode Markdown extension, so that I can maintain that piece of code without much effort.

The changed lines in this pull request (and its siblings) are not self explained and sometimes unnecessary. So if I am going to accept it, I need enough time (at least months) to really understand what it does.

Time is luxury for such side projects.

@oharboe
Copy link

oharboe commented Oct 14, 2019

@lextm Time is a luxury indeed.

The best one can do is to hope for more maintainers to present themselves(which requires time and grooming) and to try to give som quick guidance so the contributor can carry more of the load, like make the code more self explanatory so maintainability of master is not compromised.

I appreciate that the stability and maitainability of master is paramount and that you can only take delivery when PRs are truly ready and there is no followup tasks for the community.

@Ayplow
Copy link
Author

Ayplow commented Oct 16, 2019

Took a glance at the changes I made in this PR and I'm not sure what you want elaborated on. The implementation is still less than ideal (Python#setup being a constructor and the hardcoded test output), but this PR was only intended to make the tests pass in a type safe manner, while maintaining the previous interface.

I'm happy to finish this up whenever - LMK what needs changing as and when.

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

Successfully merging this pull request may close these issues.

None yet

3 participants