ronantakizawa/codereview-bench huggingface dataset:
Repository: thenewboston-developers/Website
File: src/containers/Webmap/index.tsx (TypeScript)

Code before:
import React, {FC, useEffect, useState} from 'react';
import axios from 'axios';
import {Icon} from 'leaflet';
import {Map, Marker, Popup, TileLayer} from 'react-leaflet';
import './Webmap.scss';
import * as apiEndpoints from '../../constants/api-endpoints';

export const bankIcon = new Icon({
  iconSize: [20, 20],
  iconUrl: '/bank1.svg',
});

export const validatorIcon = new Icon({
  iconSize: [20, 20],
  iconUrl: '/validator.svg',
});

const Webmap: FC = () => {
  interface APIResponse {
    account_number: number;
    city: string;
    country: string;
    default_transaction_fee: number;
    lat: number;
    lon: number;
    org: string;
    query: string;
    root_account_file: string;
  }

  const [state, setState] = useState<APIResponse[]>([]);

Review comment (suggestion):
```suggestion
import * as apiEndpoints from 'constants/api-endpoints';
```

Code after:
import React, {FC, useEffect, useState} from 'react';
import {Map, Marker, Popup, TileLayer} from 'react-leaflet';
import axios from 'axios';
import {Icon} from 'leaflet';

import bankSvg from 'assets/svgs/bank1.svg';
import validatorSvg from 'assets/svgs/validator.svg';
import * as apiEndpoints from 'constants/api-endpoints';
import {useWindowDimensions} from 'hooks';

import './Webmap.scss';

const bankIcon = new Icon({
  iconSize: [20, 20],
  iconUrl: bankSvg,
});

const validatorIcon = new Icon({
  iconSize: [20, 20],
  iconUrl: validatorSvg,
});

const Webmap: FC = () => {
  const {clientWidth} = useWindowDimensions();

  interface Node {
    account_number: number;
    city: string;
    country: string;
    default_transaction_fee: number;
    lat: number;
Repository: thenewboston-developers/Website
File: src/components/SlideUp/SlideUp.test.tsx (TypeScript)

Code before:
import React from 'react';
import {fireEvent, render, screen} from '@testing-library/react';
import '@testing-library/jest-dom/extend-expect';

import SlideUp from './index';

const slideUpRoot = document.createElement('div');
slideUpRoot.setAttribute('id', 'slide-up-root');
document.body.appendChild(slideUpRoot);

describe('SlideUp component', () => {
  test('renders with text passed in', () => {
    render(<SlideUp close={() => {}}>Hello World</SlideUp>);
    expect(screen.getByText('Hello World')).toBeTruthy();
  });

  test('calls close prop when component is clicked', () => {
    const close = jest.fn();
    const {getByRole} = render(<SlideUp close={close}>Hello World</SlideUp>);
    fireEvent.click(getByRole(/button/));
    expect(close).toHaveBeenCalledTimes(1);
  });
});


Review comment (suggestion):
lets not do this destructuring from render, and grabbing by the Role. You can pass an `data-testid` if you want to grab the button.

Code after:
import React from 'react';
import {fireEvent, render, screen} from '@testing-library/react';
import '@testing-library/jest-dom/extend-expect';

import SlideUp from './index';

const slideUpRoot = document.createElement('div');
slideUpRoot.setAttribute('id', 'slide-up-root');
document.body.appendChild(slideUpRoot);

describe('SlideUp component', () => {
  test('renders with text passed in', () => {
    render(<SlideUp close={() => {}}>Hello World</SlideUp>);
    expect(screen.getByText('Hello World')).toBeTruthy();
  });

  test('calls close prop when component is clicked', () => {
    const close = jest.fn();
    render(<SlideUp close={close}>Hello World</SlideUp>);
    fireEvent.click(screen.getByTestId('SlideUp__overlay'));
    expect(close).toHaveBeenCalled();
  });

  test('renders with className passed in', () => {
    render(<SlideUp close={() => {}} className="test" />);
    expect(screen.getByTestId('SlideUp')).toHaveClass('test');
    expect(screen.getByTestId('SlideUp__content')).toHaveClass('test__content');
  });
});
Repository: thenewboston-developers/Website
File: src/components/TotalAmount/index.tsx (TypeScript)

Code before:
import React, {FC} from 'react';
import clsx from 'clsx';
import {bemify} from '@thenewboston/utils';

import './TotalAmount.scss';

interface ComponentProps {
  amount: number;
  title: string;
  className?: string;
  dataTestId?: string;
}

const TotalAmount: FC<ComponentProps> = ({amount, className, title, dataTestId = 'TotalAmount'}) => {
  return (
    <div data-testid={dataTestId} className={clsx('TotalAmount', className)}>
      <div className={clsx('TotalAmount__title', {...bemify(className, '__title')})}>{title}</div>
      <div className={clsx('TotalAmount__amount', {...bemify(className, '__amount')})}>{amount.toLocaleString()}</div>
    </div>
  );
};

export default TotalAmount;


Review comment (suggestion):
can you add a test that will test for this `amount.toLocaleString()`? You can read about that here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toLocaleString



Code after:
import React, {FC} from 'react';
import clsx from 'clsx';
import {bemify} from '@thenewboston/utils';

import './TotalAmount.scss';

interface ComponentProps {
  amount: number;
  className?: string;
  title: string;
}

const TotalAmount: FC<ComponentProps> = ({amount, className, title}) => {
  return (
    <div className={clsx('TotalAmount', className)} data-testid="TotalAmount">
      <div className={clsx('TotalAmount__title', {...bemify(className, '__title')})} data-testid="TotalAmount__title">
        {title}
      </div>
      <div
        className={clsx('TotalAmount__amount', {...bemify(className, '__amount')})}
        data-testid="TotalAmount__amount"
      >
        {amount.toLocaleString()}
      </div>
    </div>
  );
};

export default TotalAmount;
Repository: thenewboston-developers/Website
File: src/components/Toast/index.tsx (TypeScript)

Code before:

interface ComponentProps {
  className?: string;
  type: ToastType;
}

const Toast: FC<ComponentProps> = ({children, className, type = 'warning'}) => {
  const iconType = useMemo<IconType>(() => {
    switch (type) {
      case 'success':
        return IconType.thumbsUp;
      default:
        return IconType.alertCircleOutline;
    }
  }, [type]);

  return (
    <div
      className={clsx('Toast', className, {
        [`Toast--${type}`]: true,
        ...bemify(className, `--${type}`),
      })}
      data-testid="Toast"
    >
      <Icon
        className={clsx('Toast__icon', {
          [`Toast__icon--${type}`]: true,
          ...bemify(className, '__icon'),
          ...bemify(className, `__icon--${type}`),
        })}
        icon={iconType}
        size={20}
      />
      <div className={clsx('Toast__text', {...bemify(className, '__text')})}>{children}</div>
    </div>
  );
};

export default Toast;


Review comment (suggestion):
add a `data-testid` to this Icon: `data-testid="Toast__icon"`

add a `data-testid` to the text div below: `data-testid="Toast__text"`

Code after:

interface ComponentProps {
  className?: string;
  type: ToastType;
}

const Toast: FC<ComponentProps> = ({children, className, type = 'warning'}) => {
  const iconType = useMemo<IconType>(() => {
    switch (type) {
      case 'success':
        return IconType.thumbsUp;
      default:
        return IconType.alertCircleOutline;
    }
  }, [type]);

  return (
    <div
      className={clsx('Toast', className, {
        [`Toast--${type}`]: true,
        ...bemify(className, `--${type}`),
      })}
      data-testid="Toast"
    >
      <Icon
        className={clsx('Toast__icon', {
          [`Toast__icon--${type}`]: true,
          ...bemify(className, '__icon'),
          ...bemify(className, `__icon--${type}`),
        })}
        icon={iconType}
        size={20}
        dataTestId="Toast__icon"
      />
      <div className={clsx('Toast__text', {...bemify(className, '__text')})} data-testid="Toast__text">
        {children}
      </div>
    </div>
  );
};

export default Toast;
Repository: thenewboston-developers/Website
File: src/components/Toast/Toast.test.tsx (TypeScript)

Code before:
import React from 'react';
import {render, screen} from '@testing-library/react';
import '@testing-library/jest-dom/extend-expect';

import Toast from './index';

describe('Toast component', () => {
  test('renders with children', () => {
    render(<Toast type="warning">Hello World</Toast>);

    expect(screen.getByText('Hello World')).toBeTruthy();
  });

  test('renders with default classname', () => {
    render(<Toast type="success">Test</Toast>);
    const el = screen.getByTestId('Toast');

    expect(el.className).toContain('Toast');
  });

  test('renders with success classname', () => {
    render(<Toast type="success">Test</Toast>);
    const el = screen.getByTestId('Toast');

    expect(el.className).toContain('Toast--success');
  });

  test('renders with warning classname', () => {
    render(<Toast type="warning">Test</Toast>);
    const el = screen.getByTestId('Toast');

    expect(el.className).toContain('Toast--warning');
  });

  test('renders with passed classname', () => {
    render(
      <Toast type="success" className="test">
        Test
      </Toast>,
    );
    const el = screen.getByTestId('Toast');

    expect(el.className).toContain('test');
  });

  test('renders with passed--success classname', () => {
    render(
      <Toast type="success" className="test">
        Test
      </Toast>,
    );
    const el = screen.getByTestId('Toast');

    expect(el.className).toContain('test--success');
  });

  test('renders with passed--warning classname', () => {
    render(
      <Toast type="warning" className="test">
        Test
      </Toast>,
    );
    const el = screen.getByTestId('Toast');

    expect(el.className).toContain('test--warning');
  });
});


Review comment (suggestion):
add tests here that tests for the `IconType.thumbsUp`, and `IconType.alertCircleOutline`.

One way you can do that is within your test file, import the two icons:

`
import AlertCircleOutlineIcon from 'mdi-react/AlertCircleOutlineIcon';
import ThumbsUpIcon from 'mdi-react/ThumbsUpIcon';
`

and then for the `success` icon test, something like `expect(ThumbsUpIcon).toBeEtcEtc ... expect(AlertCircleOutlineIcon).not.toBeEtcEtc`. Something along those lines

Code after:
  test('renders with passed--success classname', () => {
    render(
      <Toast type="success" className="test">
        Test
      </Toast>,
    );
    const el = screen.getByTestId('Toast');
    const icon = screen.getByTestId('Toast__icon');

    expect(el.className).toContain('Toast--success');
    expect(icon.className).toContain('Toast__icon--success');
  });

  test('renders with passed--warning classname', () => {
    render(
      <Toast type="warning" className="test">
        Test
      </Toast>,
    );
    const el = screen.getByTestId('Toast');
    const icon = screen.getByTestId('Toast__icon');

    expect(el.className).toContain('Toast--warning');
    expect(icon.className).toContain('Toast__icon--warning');
  });
});
Repository: thenewboston-developers/Website
File: src/components/ContributorTasks/ContributorTasks.test.tsx (TypeScript)

Code before:
/* eslint-disable react/jsx-props-no-spreading */
import React from 'react';
import {render, screen} from '@testing-library/react';
import '@testing-library/jest-dom/extend-expect';
import format from 'date-fns/format';

import ContributorTasks, {ContributorTasksProps} from './index';

describe('ContributorTasks', () => {
  const props: ContributorTasksProps = {
    className: 'test',
    tasks: [
      {
        amount_paid: '2800',
        completed_by: 'contributor 1',
        completed_date: new Date(Date.now()),
        issue_id: '111',
        pr_id: '222',
        repository: 'Kotlin-SDK',
        title: 'Sofware engineer',
      },
      {
        amount_paid: '2800',
        completed_by: 'contributor 2',
        completed_date: new Date(Date.now()),
        issue_id: '333',
        pr_id: '444',
        repository: 'Website',
        title: 'Front End Developer',
      },
    ],
  };

  it('renders without crashing', () => {
    render(<ContributorTasks {...props} />);

Review comment (refactor):
you can move this `props` outside of the `describe`. Put it in between the last `import` (on line 7) and before the `describe`

Code after:
import React from 'react';
import {render, screen} from '@testing-library/react';
import '@testing-library/jest-dom/extend-expect';

import ContributorTasks, {ContributorTasksProps} from '.';

const props: ContributorTasksProps = {
  className: 'test',
  tasks: [
    {
      amount_paid: '2800',
      completed_by: 'contributor 1',
      completed_date: new Date('2021/02/20'),
      issue_id: '111',
      pr_id: '222',
      repository: 'Kotlin-SDK',
      title: 'Software engineer',
    },
    {
      amount_paid: '2800',
      completed_by: 'contributor 2',
      completed_date: new Date('2021/02/24'),
      issue_id: '333',
      pr_id: '444',
      repository: 'Website',
      title: 'Front End Developer',
    },
  ],
};

const testIdTask = 'ContributorTasks';
const testIdRow = 'ContributorTasks__row';
const testIdTitle = 'ContributorTasks__task-title';
const testIdLink = 'A';
const testIdRepo = 'ContributorTasks__repository';
Repository: thenewboston-developers/Website
File: src/components/ContributorTasks/ContributorTasks.test.tsx (TypeScript)

Code before:
    });

    expect(validRepo).toBeTruthy();
  });

  it('renders rows with date compeleted', () => {
    render(<ContributorTasks {...props} />);
    const tasks = screen.getAllByTestId('ContributorTasks__date-completed');

    const validDate = tasks.every((task, i) => {
      const date = format(props.tasks[i].completed_date, 'L/d/yy');
      return task.textContent === date;
    });

    expect(validDate).toBeTruthy();
  });

  it('renders rows with valid amount', () => {
    render(<ContributorTasks {...props} />);
    const tasks = screen.getAllByTestId('ContributorTasks__amount');

    const validAmount = tasks.every((task, i) => {
      const amount = props.tasks[i].amount_paid;
      const formattedAmount = parseInt(amount, 10).toLocaleString();
      return task.textContent === `+ ${formattedAmount}`;
    });

    expect(validAmount).toBeTruthy();
  });
});


Review comment (bug):
hmm can we actually test this explicitly writing our what the string should be (instead of using logic to create the string)?

The reason is this... one of the benefits of unit tests is that it is almost self-documenting our code. But if we add too much JS logic within the test, we lose the self-documentation aspects of it.

Another reason is that it shows us exactly what the results should look like. A future developer might come in and make edits to this component, sees that this test is failing, and then just copies and pastes the logic to the test. This might defeat the purpose of the test later down the road

Code after:
  it('renders rows with valid github issue link', () => {
    render(<ContributorTasks {...props} />);
    const tasks = screen.getAllByTestId(testIdLink);

    const validHref = tasks.every((task, i) => {
      const propTask = props.tasks[i];
      const href = `https://github.com/thenewboston-developers/${propTask.repository}/issues/${propTask.issue_id}`;

      return task.getAttribute('href') === href;
    });

    expect(validHref).toBeTruthy();
  });

  it('renders rows with valid github repository repository', () => {
    render(<ContributorTasks {...props} />);
    const tasks = screen.getAllByTestId(testIdRepo);

    expect(tasks[0]).toHaveTextContent('Kotlin-SDK');
    expect(tasks[1]).toHaveTextContent('Website');
  });

  it('renders rows with date completed', () => {
    render(<ContributorTasks {...props} />);
    const tasks = screen.getAllByTestId(testIdDate);

    expect(tasks[0]).toHaveTextContent('2/20/21');
    expect(tasks[1]).toHaveTextContent('2/24/21');
  });

  it('renders rows with valid amount', () => {
    render(<ContributorTasks {...props} />);
    const tasks = screen.getAllByTestId(testIdAmount);

    expect(tasks[0]).toHaveTextContent('+ 2,800');
    expect(tasks[1]).toHaveTextContent('+ 2,800');
  });
});
Repository: thenewboston-developers/Website
File: src/components/DocWrapper/DocList/DocList.test.tsx (TypeScript)

Code before:
import React from 'react';
import {render, screen} from '@testing-library/react';
import '@testing-library/jest-dom/extend-expect';

import DocList, {DocListProps} from './index';

describe('DocList', () => {
    const props: DocListProps = {
    className: 'test-class',
    variant: 'ul',
    };

  it('renders without crashing', () => {
    render(<DocList {...props}>Hello World</DocList>);
  });

  it('renders with proper default className', () => {
    render(<DocList {...props}>Hello World</DocList>);
    const list = screen.getByTestId(`DocList--${props.variant}`);

    expect(list).toHaveClass('DocList');
  });

  it('renders unorderd list when specified in variant prop', () => {
    render(<DocList {...props}>Hello World</DocList>);
    const list = screen.getByTestId(`DocList--ul`);

    expect(list).toBeTruthy();
    expect(list).toHaveClass(`DocList--ul`);
  });

  it('renders unorderd list with passed in className', () => {
    render(<DocList {...props}>Hello World</DocList>);
    const list = screen.getByTestId(`DocList--ul`);

    expect(list).toHaveClass('test-class');
  });

  it('renders orderd list when specified in variant prop', () => {
    render(<DocList variant="ol">Hello World</DocList>);
    const list = screen.getByTestId(`DocList--ol`);

    expect(list).toBeTruthy();
    expect(list).toHaveClass(`DocList--ol`);

Review comment (suggestion):
I think you can create a `const testIdUl = 'DocList--ul` and `const testIdOl = 'DocList--ol` and reuse them all over

Code after:
import React from 'react';
import {render, screen} from '@testing-library/react';
import '@testing-library/jest-dom/extend-expect';

import DocList, {DocListProps} from './index';

const props: DocListProps = {
  className: 'test-class',
  variant: 'ul',
};

const testIdUl = 'DocList--ul';
const testIdOl = 'DocList--ol';

describe('DocList', () => {
  it('renders without crashing', () => {
    render(<DocList {...props}>Hello World</DocList>);
  });

  it('renders with proper default className', () => {
    render(<DocList {...props}>Hello World</DocList>);
    const listUl = screen.getByTestId(testIdUl);
    const listOl = screen.getByTestId(testIdUl);

    expect(listUl).toHaveClass('DocList');
    expect(listOl).toHaveClass('DocList');
  });

  it('renders unordered list when specified in variant prop', () => {
    render(<DocList {...props}>Hello World</DocList>);
    const list = screen.getByTestId(testIdUl);

    expect(list).toBeTruthy();
    expect(list).toHaveClass(testIdUl);
  });

  it('renders unordered list with passed in className', () => {
    render(<DocList {...props}>Hello World</DocList>);
    const list = screen.getByTestId(testIdUl);

    expect(list).toHaveClass('test-class');
    expect(list).toHaveClass('test-class--ul');
  });
Repository: thenewboston-developers/Website
File: src/components/Tabs/Tabs.test.tsx (TypeScript)

Code before:
import React from 'react';
import {fireEvent, render, screen} from '@testing-library/react';
import '@testing-library/jest-dom/extend-expect';

import Tabs from './index';

const TestTabs = [
  {
    component: <span>Tab 1</span>,
    label: 'FirstTab',
  },
  {
    component: <span>Tab 2</span>,
    label: 'SecondTab',
  },
];

describe('Tabs component', () => {
  test('renders with default testId Tabs', () => {
    render(<Tabs tabs={TestTabs} latestReleaseNumber={1} />);
    const tabs = screen.getByTestId('Tabs');
    expect(tabs).toBeTruthy();
  });

  test('renders with className Tabs', () => {
    render(<Tabs tabs={TestTabs} latestReleaseNumber={1} />);
    const tabs = screen.getByTestId('Tabs');
    expect(tabs).toHaveClass('Tabs');
  });

  test('renders with Download__latest-version', () => {
    render(<Tabs tabs={TestTabs} latestReleaseNumber={1} />);

Review comment (suggestion):
Also, create a `baseProps` or `props` object here and use it for props.

That way, if we decide to change props later, we can do it in one place.

Check MarketingCard.test.tsx or Layout.test.tsx, for reference.



Code after:
import React from 'react';
import {fireEvent, render, screen} from '@testing-library/react';
import '@testing-library/jest-dom/extend-expect';

import Tabs, {Tab, TabsProps} from '.';

const TestTabs: Tab[] = [
  {
    component: <span>Tab 1</span>,
    label: 'FirstTab',
  },
  {
    component: <span>Tab 2</span>,
    label: 'SecondTab',
  },
];

const baseProps: TabsProps = {
  latestReleaseNumber: 1,
  tabs: TestTabs,
};

describe('Tabs component', () => {
  test('renders without crashing', () => {
    render(<Tabs {...baseProps} />);

    const tabs = screen.getByTestId('Tabs');
    expect(tabs).toBeTruthy();
  });

  test('renders with default className', () => {
    render(<Tabs {...baseProps} />);
Repository: thenewboston-developers/Website
File: src/components/MenuGroup/MenuGroup.test.tsx (TypeScript)

Code before:
import React from 'react';
import {BrowserRouter as Router} from 'react-router-dom';
import {render, screen, fireEvent} from '@testing-library/react';
import '@testing-library/jest-dom/extend-expect';

import MenuGroup from './index';

describe('MenuGroup component', () => {
  test('renders without crashing', async () => {
    render(
      <Router>
        <MenuGroup title="Test" urlBase="Test" />
      </Router>,
    );

    expect(screen.getByTestId('MenuGroup')).toBeTruthy();
  });

  test('renders with customTestId "Custom" without crashing', async () => {
    render(
      <Router>
        <MenuGroup dataTestId="Custom" title="Test" urlBase="Test" />
      </Router>,
    );

    expect(screen.getByTestId('Custom')).toBeTruthy();
  });

  test('renders with title', async () => {
    render(
      <Router>
        <MenuGroup title="Test" urlBase="Test" />
      </Router>,
    );
    expect(screen.getByText('Test')).toBeTruthy();
  });


Review comment (suggestion):
No passing props like this. Create an object before `describe` and use that. Just like I pointed out in the last PR.

Check this commit:
[Tabs tests](https://github.com/thenewboston-developers/Website/commit/5a9442bb197d32a455d35dac5b0ad61beb3a0f78)

Code after:
import React from 'react';
import {Router} from 'react-router-dom';
import {render, screen, fireEvent} from '@testing-library/react';
import '@testing-library/jest-dom/extend-expect';
import {createMemoryHistory} from 'history';

import MenuGroup from '.';

const baseProps = {
  children: (
    <>
      <div>Child 1</div>
      <div>Child 2</div>
    </>
  ),
  title: 'Test',
  urlBase: 'test',
};

describe('MenuGroup component', () => {
  it('renders without crashing', () => {
    const history = createMemoryHistory();
    render(
      <Router history={history}>
        <MenuGroup {...baseProps} />
      </Router>,
    );

    expect(screen.getByTestId('MenuGroup')).toBeTruthy();
  });

  it('renders with role when passed in', () => {
    const history = createMemoryHistory();
    const props = {...baseProps, role: 'group'};
    render(
      <Router history={history}>
        <MenuGroup {...props} />