Freecodecamp: [curriculum] Commented code is getting accepted in a challenge

Created on 1 Sep 2019  路  7Comments  路  Source: freeCodeCamp/freeCodeCamp

Describe your problem and how to reproduce it:
After commenting the code in a challenge, the tests are passing. I think the issue is with this particular challenge only as I have solved many challenges and I haven't observed this bug in any of them.

bug

Add a Link to the page with the problem:
https://www.freecodecamp.dev/learn/responsive-web-design/responsive-web-design-principles/make-typography-responsive

Tell us about your browser and operating system:

  • Browser Name: Chrome
  • Browser Version: 76.0.3809.132 (Official Build) (32-bit)
  • Operating System: Windows 10 Pro
help wanted client learn bug

Most helpful comment

@vkWeb, @raisedadead, @RandellDawson, How do this look? The idea is to provide a challenge test utility to replace the need upon horrible RegEx tests.

I have this passing locally as a POC.

describe('HtmlValidator', () => {
  describe('html validation utilities', () => {
    const testDom = `
      <header>
        <h1>Hello World</h1>
        <h2>CatPhotoApp</h2>
        <div id='hero'>
          <img src='hero-splash.png'/>
        </div>
      </header>
      <main>
        <p>
          <span class='bold-text'>Some bold text</span>
          . A bunch more text
        <!-- This is a comment -->
        </p>
      </main>`;

    it('validates if an element exists in the DOM under test', () => {
      expect.assertions(6);
      const validator = new HtmlValidator(testDom);

      expect(validator.domNodeExists('.bold-text')).toBe(true);
      expect(validator.domNodeExists('#hero')).toBe(true);
      expect(validator.domNodeExists('header #hero')).toBe(true);
      expect(validator.domNodeExists('header > #hero')).toBe(true);
      expect(validator.domNodeExists('.not-here')).toBe(false);
      expect(validator.domNodeExists('#not-here')).toBe(false);
    });

    it('validates if an element has text', () => {
      expect.assertions(5);
      const validator = new HtmlValidator(testDom);

      expect(validator.elementHasText('main > p', 'more text')).toBe(true);
      expect(validator.elementHasText('main > p', 'MORE text')).toBe(true);
      expect(validator.elementHasText('main > p', 'Some bold text')).toBe(true);

      expect(validator.elementHasText('main > p', 'Not here')).toBe(false);
      expect(validator.elementHasText('main > p', 'a comment')).toBe(false);
    });

    it('validates DOM comments', () => {
      const validator = new HtmlValidator(testDom);

      const noCommentValidator = new HtmlValidator('<h1>Hello, World!</h1>');
      expect(validator.domHasComments()).toBe(true);
      expect(noCommentValidator.domHasComments()).toBe(false);
    });

    it('validates comment text', () => {
      const validator = new HtmlValidator(testDom);

      expect(validator.domCommentsHasText('This is a comment')).toBe(true);
      expect(validator.domCommentsHasText('Nope')).toBe(false);
    });
  });
});

Screenshot 2019-09-15 at 14 19 32

All 7 comments

This is not unique to the beta site. Many of the challenges don't ignore comments. We have a helper function which can be applied to the tests which removes comments. We can not use it across all challenges, because there are challenges which involve validating the comments.

Since there are only a few challenge tests where we need to be able to check the actual HTML, CSS, or JavaScript comments, maybe we can figure out a way to pass an extra yaml property named checkComments with a value of true only when comments should not be removed from a specific testString's use of the code variable. That way, the default behavior would always be to remove comments from a test. It might be simpler to just add the checkComments to the frontMatter of the challenge, so all the tests of a challenge would not remove the comments and the testStrings could be adjusted to account for the comments not being removed from the code variable.

I do not like the idea of changing the schema again though. What we really need is a parser to sanitize comments.

I do not like the idea of changing the schema again though. What we really need is a parser to sanitize comments.

Some of the challenges are actually checking the comments, so you can not just sanitize all the challenges' comments.

Did someone say hast?

Ooops, been a while since I have been on GitHub.

Using an AST will help remove those awful regex tests that many of the older challenges rely on. The challenge-markdown-parser is built with unist.

Is wouldn't take much to create a library of helper functions to use in tests.

@vkWeb, @raisedadead, @RandellDawson, How do this look? The idea is to provide a challenge test utility to replace the need upon horrible RegEx tests.

I have this passing locally as a POC.

describe('HtmlValidator', () => {
  describe('html validation utilities', () => {
    const testDom = `
      <header>
        <h1>Hello World</h1>
        <h2>CatPhotoApp</h2>
        <div id='hero'>
          <img src='hero-splash.png'/>
        </div>
      </header>
      <main>
        <p>
          <span class='bold-text'>Some bold text</span>
          . A bunch more text
        <!-- This is a comment -->
        </p>
      </main>`;

    it('validates if an element exists in the DOM under test', () => {
      expect.assertions(6);
      const validator = new HtmlValidator(testDom);

      expect(validator.domNodeExists('.bold-text')).toBe(true);
      expect(validator.domNodeExists('#hero')).toBe(true);
      expect(validator.domNodeExists('header #hero')).toBe(true);
      expect(validator.domNodeExists('header > #hero')).toBe(true);
      expect(validator.domNodeExists('.not-here')).toBe(false);
      expect(validator.domNodeExists('#not-here')).toBe(false);
    });

    it('validates if an element has text', () => {
      expect.assertions(5);
      const validator = new HtmlValidator(testDom);

      expect(validator.elementHasText('main > p', 'more text')).toBe(true);
      expect(validator.elementHasText('main > p', 'MORE text')).toBe(true);
      expect(validator.elementHasText('main > p', 'Some bold text')).toBe(true);

      expect(validator.elementHasText('main > p', 'Not here')).toBe(false);
      expect(validator.elementHasText('main > p', 'a comment')).toBe(false);
    });

    it('validates DOM comments', () => {
      const validator = new HtmlValidator(testDom);

      const noCommentValidator = new HtmlValidator('<h1>Hello, World!</h1>');
      expect(validator.domHasComments()).toBe(true);
      expect(noCommentValidator.domHasComments()).toBe(false);
    });

    it('validates comment text', () => {
      const validator = new HtmlValidator(testDom);

      expect(validator.domCommentsHasText('This is a comment')).toBe(true);
      expect(validator.domCommentsHasText('Nope')).toBe(false);
    });
  });
});

Screenshot 2019-09-15 at 14 19 32

This would be really nice to be able to update those quirky tests. Can you please throw in you changes in a PR?

Was this page helpful?
0 / 5 - 0 ratings