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

Remove fixed bugs from test\functional\test_site\bugs\index.md` #1148

Merged
merged 18 commits into from
Apr 2, 2020

Conversation

Tejas2805
Copy link
Contributor

@Tejas2805 Tejas2805 commented Mar 25, 2020

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [X] Other, please explain:
The test\functional\test_site\bugs\index.md file has some bugs listed which have already been fixed already. Let's remove them.

Fixes #1140

What is the rationale for this request?
Some bugs in the open bugs page have already been fixed. Therefore I would be good to tidy up and remove fixed bugs from test\functional\test_site\bugs\index.md
In order to make sure that these we do not regress and have these bugs again, add these cases as tests in appropriate test_site markdown files, or create new markdown files if nothing suitable is present.

What changes did you make? (Give an overview)

Added test file for the bugs fixed

Provide some example code that this change will affect:
N.A.

Is there anything you'd like reviewers to focus on?
Nothing specifically

Testing instructions:
N.A.

Proposed commit message: (wrap lines at 72 characters)

Some bugs in the open bugs page have been fixed already but they
are still present in the page.

Let's remove the fixed bugs from the open bugs page.  

Let's also add a test case for these bugs, if not added already, 
in the appropriate test_site markdown files. This will ensure 
we don't regress and have these bugs again. 

* 'master' of https://github.com/MarkBind/markbind:
  Update tests
  Allow using 'none' footer attribute in frontmatter (MarkBind#1002)
  Support line numbers for code blocks (MarkBind#991)
  2.11.0
  Update test files due to changes in PR MarkBind#982
  Update vue-strap version to v2.0.1-markbind.36
  Make highlighting bold (MarkBind#1045)
  Support markdown for header attr in dropdown (MarkBind#1029)
  Add '_site' to the ignored folders in site.json (MarkBind#1046)
  Use path.join instead of string interpolation (MarkBind#1052)
  Implement box markdown header attributes parsing (MarkBind#1025)
  Make the position of top navbar fixed (MarkBind#982)
  Exclude *.md files from being copied over on build (MarkBind#1010)

# Conflicts:
#	docs/css/main.css
* 'master' of https://github.com/MarkBind/markbind:
  2.12.0
  Update outdated test files
  Update vue-strap version to v2.0.1-markbind.37
  Fix refactor to processDynamicResources (MarkBind#1092)
  Implement lazy page building for markbind serve (MarkBind#1038)
  Add warnings for conflicting/deprecated component attribs (MarkBind#1057)
  Allow changing parameter properties (MarkBind#1075)
  Custom timezone for built-in timestamp (MarkBind#1073)
  Fix reload inconsistency when updating frontmatter (MarkBind#1068)
  Implement an api to ignore content in certain tags (MarkBind#1047)
  Enable AppVeyor CI (MarkBind#1040)
  Add heading and line highlighting to code blocks (MarkBind#1034)
  Add dividers and fix bug in siteNav (MarkBind#1063)
  Fixed navbar no longer covers modals (MarkBind#1070)
  Add copy code-block plugin (MarkBind#1043)
  Render plugins on dynamic resources (MarkBind#1051)
  Documentation for Implement no-* attributes for <box>  (MarkBind#1042)
  Migrate to bootstrap-vue popovers (MarkBind#1033)
  Refactor preprocess and url processing functions (MarkBind#1026)
  Add pageNav to Using Plugins Page (MarkBind#1062)
* 'master' of https://github.com/MarkBind/markbind:
  Add example for using multiple features in code-blocks (MarkBind#1102)
* 'master' of https://github.com/MarkBind/markbind:
  Update test files (MarkBind#1138)
  Remove OK button from modals (MarkBind#1134)
  Add start from line number functionality to code blocks (MarkBind#1115)
  Allow special tags to be self-closing (MarkBind#1101)
  Simplify baseUrl resolving process (MarkBind#1087)
  Remove redundant file writing (MarkBind#1090)
  Bump acorn from 5.7.3 to 5.7.4 (MarkBind#1121)
  Bump acorn from 7.1.0 to 7.1.1 in /src/lib/markbind (MarkBind#1120)
  Unify markdown-it parser variants (MarkBind#1056)
  Remove dynamic include feature (MarkBind#1037)
  Fix flex shrink not applying in content wrapper (MarkBind#1135)
  Escape Nunjucks for special tags (MarkBind#1049)
  Update documentation on icon slot for boxes (MarkBind#1123)
@Tejas2805
Copy link
Contributor Author

@nbriannl The last issue was closed but hasn't been fixed. Should I keep it or remove it?

@nbriannl
Copy link
Contributor

@Tejas2805 Which one?

@Tejas2805
Copy link
Contributor Author

The one which I didn't remove - #147

@nbriannl
Copy link
Contributor

nbriannl commented Mar 25, 2020

@Tejas2805 In my opinion, i think you can remove it.

And since the issue is closed, and we've pretty much given up to solve it, this bug is considered closed. It seems that there's corresponding documentation informing users about this. So we're alright on that end.

Oh, and could you check if these behaviors have a corresponding test in any of the test site files?

I was thinking, instead of removing, I think it would be more suitable to move them to the corresponding test files. If there isn't any existing test file covering the related feature, then you can create a new one.

@Tejas2805
Copy link
Contributor Author

@nbriannl Did you have something like this in mind?

Copy link
Contributor

@nbriannl nbriannl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. What I had in mind. Having looked through other test files, functional testing description should be phrased as an ability to achieve something i.e. '... should be ... ', ' should still' etc.

I gave my suggestions below.

@@ -0,0 +1,4 @@
**Support Multiple Modals**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'Multiple inclusions of a modal should be supported'

test/functional/test_site/requirements/createModal.md Outdated Show resolved Hide resolved
@@ -0,0 +1,8 @@
**Test Trigger Attribute is Honored**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps 'Popover initiated by trigger should honor trigger attribute'

@Tejas2805
Copy link
Contributor Author

Tejas2805 commented Mar 26, 2020

Btw how to generate the corresponding HTML files?

npm run updatetest or npm run updatetestwin

@Tejas2805 Tejas2805 requested a review from nbriannl March 26, 2020 15:42
@Tejas2805
Copy link
Contributor Author

@nbriannl I tried running the commands. But no new files were created. Only the old ones got updated

@Tejas2805
Copy link
Contributor Author

@marvinchin Need your review on this.

@nbriannl
Copy link
Contributor

nbriannl commented Mar 27, 2020

@nbriannl I tried running the commands. But no new files were created. Only the old ones got updated

I see, my bad. I guess we need documentation regarding updating tests.

  1. Open test\functional\test_site\site.json
  2. To include a new page, eg: testIncludeMultipleModals.md, add it to the pages array.
"pages": [
    {
      "src": "index.md",
      "title": "Hello World",
      "frontmatter": {
        "frontMatterOverrideProperty": "Overridden by front matter override",
        "globalAndFrontMatterOverrideProperty":  "Overridden by front matter override"
      }
    },
    ...
    {
      "src": "testLayouts.md",
      "title": "Hello World"
    },
    {,
      "src": "testIncludeMultipleModals.md",
      "title": [some title you see fit]
    },
    ....
  1. Then you should be able to update test sites.

And just an advice, you can ping marvin via the top right reviewers section as well. 🙂

@Tejas2805
Copy link
Contributor Author

And just an advice, you can ping marvin via the top right reviewers section as well. 🙂

Don't seem to have access to do that. This is why have to tag him everytime

@Tejas2805
Copy link
Contributor Author

I guess we need documentation regarding updating tests.

Want me to do it in Separate PR or can do in this one?

@nbriannl
Copy link
Contributor

Don't seem to have access to do that. This is why have to tag him everytime

Oh! Today i learned, sorry for the misunderstanding. 😞

Want me to do it in Separate PR or can do in this one?

A separate PR (an issue) would be better.

Copy link
Contributor

@marvinchin marvinchin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for cleaning this up, it mostly looks good to me! Just some minor nits.

Also, sorry I didn't realise you didn't have permissions to request reviews! Let me look at this 🙏

@@ -4,37 +4,4 @@ header: header.md
</frontmatter>

<div class="website-content">

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like we now have an empty bugs file 🎉

Should we leave a comment here so that future contributors know what this file is for, and where to place their bugs?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should probably be something like a template:

**Bug Description**
<a href=LINK_TO_MARKBIND_ISSUE>Issue #XX</a>

Repro:

Repro Steps...

@@ -0,0 +1,8 @@
**Popover initiated by trigger should honor trigger attribute**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely just a nit, but testTriggerAttributeHonor.md sounds a little odd to me. What do you think of testTriggerAttributeHonor.md -> testPopoverTrigger.md?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya, agreed. Makes sense

@@ -0,0 +1,5 @@
<!-- Used in test_site/testIncludeMultipleModals.md---->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

createModal.md sounds a little misleading here, since this isn't a function that creates a modal or anything like that. Shall we just name this testModal.md?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay sure

Tejas2805 and others added 3 commits March 29, 2020 06:35
…bind into remove-fixed-bugs

* 'remove-fixed-bugs' of https://github.com/Tejas2805/markbind:
  Docs: Add contributing.md (MarkBind#1139)
  Show pointer and use underline dashed for click trigger (MarkBind#1111)
  Support variables to be defined as by JSON (MarkBind#1117)
  Allow an array for specifying page src or glob (MarkBind#1118)
  Code blocks: Implement inline markdown support for heading (MarkBind#1137)
  Fix lazy reload for urls without index (MarkBind#1110)
  Changes remaining references from filterTags to tags (MarkBind#1149)
@Tejas2805
Copy link
Contributor Author

@nbriannl @marvinchin Need your reviews on this.

Copy link
Contributor

@nbriannl nbriannl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nbriannl nbriannl requested a review from ang-zeyu April 1, 2020 09:05
@nbriannl
Copy link
Contributor

nbriannl commented Apr 1, 2020

Hi @Tejas2805 do you mind to put a commit message for this PR. I'll have a look at it one more time. 🙂

@Tejas2805
Copy link
Contributor Author

@nbriannl Done!

@nbriannl
Copy link
Contributor

nbriannl commented Apr 1, 2020

LGTM once more. You addressed all of @marvinchin's comment. I'll leave it until tmr morning for any dev to comment on this, then i'll merge.

@nbriannl nbriannl added the pr.CodeMaintenance 🛠 DevOps, refactoring, etc label Apr 1, 2020
Copy link
Contributor

@marvinchin marvinchin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

@marvinchin marvinchin added this to the v2.13.2 milestone Apr 2, 2020
@marvinchin marvinchin merged commit 7ec7bc4 into MarkBind:master Apr 2, 2020
Tejas2805 added a commit to Tejas2805/markbind that referenced this pull request Apr 9, 2020
* 'master' of https://github.com/MarkBind/markbind: (41 commits)
  Document adding new site content in DG (MarkBind#1153)
  Add relative date feature (MarkBind#908)
  Use <br> to separate lines of code block (MarkBind#1176)
  Parse popovers for footnotes (MarkBind#1155)
  Resolve comments
  Expand test extensions and fix whitespace checks (MarkBind#1156)
  Address comments
  Upgrade js-beautify and provide option to turn it off (MarkBind#1116)
  Normalize inline puml line ending before hashing (MarkBind#1174)
  Update tests (MarkBind#1178)
  Remove fixed bugs from test\functional\test_site\bugs\index.md` (MarkBind#1148)
  Fix bug in Search for UG and DG (MarkBind#1147)
  Add inline puml support (MarkBind#1100)
  Fix hrefs for headings with angular brackets (MarkBind#1089)
  Update tests for 2.13.1 (MarkBind#1169)
  2.13.1
  Update vue-strap version to v2.0.1-markbind.39
  Fix fontawsome icons don't show underlines to indicate modal/tooltip (MarkBind#1133)
  2.13.0
  Update test files
  ...
@Tejas2805 Tejas2805 deleted the remove-fixed-bugs branch April 11, 2020 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr.CodeMaintenance 🛠 DevOps, refactoring, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move fixed bugs out of test\functional\test_site\bugs\index.md
3 participants